prometheusremotewrite: Don't generate target_info unless at least one identifying label is defined

[aknuds1]

Description:
I propose modifying the Prometheus remote write translator such that it will only generate the target_info metric if at least one identifying label is defined, i.e. either job or instance (or both). The rationale is that without any identifying labels, target_info is useless (you can't join against it).

See Slack thread for background.

Testing:
I've modified the addResourceTargetInfo tests to reflect that at least one identifying label is required, and added some new ones to cover the different cases. I also modify a couple of prometheusremotewriteexporter tests that are affected by this change, as they don't define the identifying resource attributes.

Documentation:

[dashpole]

May I ask which receiver you are using? OTel SDKs default service.name to unknown_service if it isn't provided by users.

We already have a config knob for disabling target_info. Does that not meet your needs?

[aknuds1]

@dashpole I'm not aware of a specific component exhibiting this problem. I'm approaching this from the Prometheus perspective (I'm one of the Prometheus OTLP maintainers), where I think there should be a guarantee that target_info as generated by the OTel to Prometheus converter has the minimum identifying label set (i.e., job, as instance can't guaranteed with service.instance.id not being mandatory). Having this guarantee is useful when explicitly persisting the identifying metrics of info metrics (e.g. target_info) to the TSDB in Prometheus, which is something I'm prototyping.

We already have a config knob for disabling target_info. Does that not meet your needs?

I'm not looking to disable target_info, the motivation is to define that service.name (and thus the job label) is mandatory when generating target_info.

Considering that service.name is mandatory according to the spec (@jpkrohling says that an OTel metrics write payload should be considered invalid if it's missing), what would stand in the way of enforcing it in the OTel -> Prometheus converter?

[dashpole]

Thanks for the context.

Considering that service.name is mandatory according to the spec (@jpkrohling says that an OTel metrics write payload should be considered invalid if it's missing)

My understanding is that service.name is mandatory for OTel SDKs, but not for the protocol. It comes from here: https://opentelemetry.io/docs/specs/semconv/resource/#service, which doesn't apply to the protocol.

That being said, I support requiring join keys for target_info to make it useful. I could see us doing either:

1. Require either job or instance to be set
2. Require job to be set
3. Require instance to be set
4. Require both job and instance to be set

(1) or (2) both seem reasonable to me. It would be nice to document the behavior in the conversion spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#resource-attributes-1

[aknuds1]

Thanks for the feedback @dashpole! Let me confer with @jpkrohling.

[aknuds1]

That being said, I support requiring join keys for target_info to make it useful. I could see us doing either:

Require either job or instance to be set
Require job to be set
Require instance to be set
Require both job and instance to be set

@dashpole the feedback from @jpkrohling is a preference for option 1 or 2, slightly leaning towards option 1, as it's more lenient. What I am wondering though, is whether we should have a dummy value if one of the two labels is missing? Since the expectation in Prometheus is for target_info to have both job and instance labels to join against. WDYT?

[aknuds1]

I've revised the PR as per our Slack discussion @dashpole, thanks for all the feedback.

[aknuds1]

CI failure looks like a flake.

[Aneurysm9]

We cannot ignore existing service names.

[aknuds1]

@Aneurysm9 applied your suggestions, PTAL :)