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

[mx-psi]

Description:

Do not add host.name for Prometheus receiver metrics that come from scraping a localhost or unspecified (0.0.0.0) target.

Link to tracking Issue: Fixes #6465.

Testing: Amended unit test. Ran example on linked issue to manually check this fixed the problem:

logging exporter when running example on linked issue

Resource labels:
     -> service.name: STRING(otelcol)
     -> job: STRING(otelcol)
     -> instance: STRING(0.0.0.0:8888)
     -> port: STRING(8888)
     -> scheme: STRING(http)
InstrumentationLibraryMetrics #0
InstrumentationLibrary  
Metric #0
Descriptor:
     -> Name: otelcol_exporter_enqueue_failed_log_records
     -> Description: Number of log records failed to be added to the sending queue.
     -> Unit: 
     -> DataType: Sum
     -> IsMonotonic: true
     -> AggregationTemporality: AGGREGATION_TEMPORALITY_CUMULATIVE
NumberDataPoints #0
Data point attributes:
     -> exporter: STRING(logging)
     -> service_instance_id: STRING(dd14a920-f022-4e27-a638-3f2dd8327716)
     -> service_version: STRING(latest)
StartTimestamp: 2021-12-06 11:46:20.958 +0000 UTC
Timestamp: 2021-12-06 11:46:20.958 +0000 UTC
Value: 0.000000

[jpkrohling]

What's up with the instance value? Shouldn't it be 127.0.0.1 as well?

     -> instance: STRING(0.0.0.0:8888)

[mx-psi]

What's up with the instance value? Shouldn't it be 127.0.0.1 as well?

I don't have a strong opinion about this. I see a couple arguments in favor of keeping it as 0.0.0.0:8888: the instance is not a semantic convention, so the 0.0.0.0 is not incorrect in that sense and its useful to have the original value somewhere, but I also realize having host.name + port == instance seems like an useful property.

[jpkrohling]

I'm approving, as I also don't see a problem with instance having 0.0.0.0. If you decide to leave it as it is, let me know and I'll merge this.

[mx-psi]

I would leave it as it is, but maybe let's wait a bit in case @Aneurysm9 or @dashpole have a different opinion, since they are CODEOWNERS?

[bertysentry]

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.

[mx-psi]

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?

[dashpole]

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

[mx-psi]

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.

[dashpole]

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

[bertysentry]

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.

[mx-psi]

Removing it only when its a localhost equivalent?

Only when localhost or equivalent, yes

[dashpole]

@Aneurysm9 do we need to be able to distinguish between 0.0.0.0 and 127.0.0.1 for prom compliance reasons? I.e. If we rename 0.0.0.0 to 127.0.0.1, we presumably need to do the reverse in the prom remote write exporter to match the prometheus server. But if 0.0.0.0 and 127.0.0.1 are both valid, then we wouldn't be able to do that.

Edit: Looking more closely, we preserve instance anyways, so the above doesn't apply here.

[jpkrohling]

From what I understood, there's an agreement then in removing the label when the value is localhost (127.*, 0.0.0.0), right?

[mx-psi]

Updated PR diff, name and description after discussion above. PTAL!