[exporter/prometheusremotewrite] support disabling target_info metric

[hardproblems]

Description:
Add the ability to turn off the target_info metric to address duplicate series / out of order samples issue below

Link to tracking Issue:
#12300

Testing:

• added unit test in pkg/translator/prometheusremotewrite/helper.go

Documentation:

• added a comment in config.go
• added explanation for the param in README

@dashpole @Aneurysm9 @bogdandrutu @gouthamve @jmacd @jsuereth

[jmacd]

Speculating that target_info has a bug.

#12300 (comment)

I would probably add to the current specification that if the target_info has no fields, it should be automatically omitted.

[jmacd]

Shouldn't you have the option remove this at the source, i.e., in the prometheus receiver? You've described a single-writer violation happening and I'd rather it be removed before it enters the OTLP pipeline. Otherwise, receivers other than PRW will face the same single-writer violations.

[dashpole]

It is possible today to droptarget_info in the prometheus receiver using metric_relabel_configs:

metric_relabel_configs:
- source_labels: [__name__]
  regex: target_info
  action: drop

There is an added issue that we add "implicit" resource attributes (e.g. net.host.name) from instance (described in #9967) which produces an outgoing target_info metric even if you dropped the incoming target_info in the prometheus receiver. It is also common for users to use automatic resource detectors (e.g. the gcp detector) to add resource information. This would end up appearing in the target_info metric (which is useful!) and may cause the target_info collision if job and instance are not identifying.

[jmacd]

which produces an outgoing target_info metric even if you dropped the incoming target_info in the prometheus receiver

This sounds like another reason to move target_info generation to the receiver. Subsequent resource detectors and processors can edit the resource, but the decision to have or not have a target_info would be up to the receiver.

[hardproblems]

Shouldn't you have the option remove this at the source, i.e., in the prometheus receiver? You've described a single-writer violation happening and I'd rather it be removed before it enters the OTLP pipeline. Otherwise, receivers other than PRW will face the same single-writer violations.

We're using the otlp receiver. I can try using resource attributes processor to drop otel sdk added resources attributes. iirc job/instance labels are likely added by the exporter so no way to drop those using processors.

[jmacd]

job/instance labels are likely added by the exporter so no way to drop those using processors.

This sounds like a problem worth fixing. When target_info has only two attributes "job" and "instance" it is redundant with up as generated by the prometheus receiver. I would support a PRWexporter option to not generate target_info, therefore, when it contains only job and instance.

[dashpole]

This sounds like a problem worth fixing. When process_info has only two attributes "job" and "instance" it is redundant with up as generated by the prometheus receiver. I would support a PRWexporter option to not generate process_info, therefore, when it contains only job and instance.

Do you mean target_info instead of process_info? If so, I agree. That was the initial intention, but it doesn't look like it was implemented that way. We should be checking if there are no attributes after the RemoveIf that filters out job, instance.

[jmacd]

@dashpole yes, I've corrected the comment. Thanks.

[hardproblems]

@mx-psi updated target_info config to use a struct. PTAL 🙇

[hardproblems]

@Aneurysm9 PTAL as well 🙇

[dashpole]

config_test.go:98:16: cannot use (TargetInfo literal) (value of type TargetInfo) as *TargetInfo value in struct literal (typecheck)
			TargetInfo: TargetInfo{

[hardproblems]

@dashpole @mx-psi @Aneurysm9 can I get some help merging this? I'm not authorized to merge and I have to keep merging main to this while waiting for a stamp to trigger CI checks.

[bogdandrutu]

@dashpole there are lint errors in this PR.

[dashpole]

@bogdandrutu agh, sorry. I thought the author had fixed it, but I had to approve the tests.

[dashpole]

@hardproblems you can hold off rebasing on main for a bit (unless there is a conflict). The maintainers can do that before they merge. Its hard to tell if your tests are passing or not...

[hardproblems]

@dashpole @Aneurysm9 @mx-psi @bogdandrutu what's the process for getting this merged? I've looked through the docs https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md and https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/release.md and am still unclear what the process is after the code reviews are done and the CI checks appear to be passing.