Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[receiver/prometheusreceiver] Do not add host.name to metrics from localhost/unspecified targets #6476

Merged
merged 6 commits into from
Dec 6, 2021
Merged

[receiver/prometheusreceiver] Do not add host.name to metrics from localhost/unspecified targets #6476

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
019c3fe
[receiver/prometheusreceiver] Set host.name to 127.0.0.1 for 0.0.0.0 …
mx-psi Dec 1, 2021
ecb320e
[receiver/prometheusreceiver] Do not include localhost-like or unspec…
mx-psi Dec 6, 2021
0c4a905
Fix unit tests; add localhost unit test
mx-psi Dec 6, 2021
f4da92f
Rename to `isDiscernibleHost`
mx-psi Dec 6, 2021
832eeab
Fix unit test panicking
mx-psi Dec 6, 2021
d2c73c1
Address review comments
mx-psi Dec 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions receiver/prometheusreceiver/internal/prom_to_otlp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,24 @@ import (
conventions "go.opentelemetry.io/collector/model/semconv/v1.5.0"
)

const (
localHostIPv4 = "127.0.0.1"
anyIPIPv4 = "0.0.0.0"
)

func sanitizeHost(host string) string {
if host == anyIPIPv4 {
return localHostIPv4
}
return host
}

func createNodeAndResourcePdata(job, instance, scheme string) pdata.Resource {
host, port, err := net.SplitHostPort(instance)
if err != nil {
host = instance
}
host = sanitizeHost(host)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to remove the host name Attribute when its value is 0.0.0.0, localhost, 127.0.0.1 or ::1 (or any variation of this) and let the resourcedetection processor do the job or setting this attribute properly (with the actual host name of the system).

Because eventually, the metric is going to be stored somewhere else, where localhost will be either meaningless or misleading.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I certainly would prefer that (e.g. the way we as a vendor use host.name makes it so localhost and similar attributes are practically useless), but I was assuming these attributes were necessary for something Prometheus-related.

Maybe @dashpole can comment on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are using the resource detection processor, it overrides the existing values by default anyways. I double-checked, and host.name isn't used for anything prometheus-related

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are using the resource detection processor, it overrides the existing values by default anyways.

There are valid use cases where you don't want the resourcedetectionprocessor to override the hostname (e.g. chaining multiple Collectors), or maybe you can't/don't want to use it at all (e.g. your Collector distro doesn't support it or you don't want the performance hit).

I double-checked, and host.name isn't used for anything prometheus-related

Personally I would prefer removing it then, if everyone is on board with this. I agree with @bertysentry that having a localhost-like value for host.name is, while spec-compliant, not very useful in practice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing it only when its a localhost equivalent? Or removing it entirely?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resourcedetectionprocessor has an override option that can be set to false to prevent it from overwriting attributes that are already set.

Removing the host.name attributes in prometheusreceiver when its value is localhost (or any variation of localhost, incl. 0.0.0.0) will then work very well in combination with resourcedetectionprocessor (and override: false).

Typical example: the internal otelcol Prometheus exporter, whose metrics are currently attached to 0.0.0.0, which are not usable once these metrics from several collectors are aggregated into a Prometheus server.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing it only when its a localhost equivalent?

Only when localhost or equivalent, yes

resource := pdata.NewResource()
attrs := resource.Attributes()
attrs.UpsertString(conventions.AttributeServiceName, job)
Expand Down
7 changes: 7 additions & 0 deletions receiver/prometheusreceiver/internal/prom_to_otlp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,13 @@ func TestCreateNodeAndResourcePromToOTLP(t *testing.T) {
"job", "", "", "http", "",
}),
},
{
name: "0.0.0.0 address",
job: "job", instance: "0.0.0.0:8888", scheme: "http",
want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{
"job", "0.0.0.0:8888", "127.0.0.1", "http", "8888",
}),
},
}

for _, tt := range tests {
Expand Down
1 change: 1 addition & 0 deletions receiver/prometheusreceiver/internal/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ func createNodeAndResource(job, instance, scheme string) (*commonpb.Node, *resou
if err != nil {
host = instance
}
host = sanitizeHost(host)
node := &commonpb.Node{
ServiceInfo: &commonpb.ServiceInfo{Name: job},
Identifier: &commonpb.ProcessIdentifier{
Expand Down