-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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.
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
prometheusremotewrite: Don't generate target_info
unless at least one identifying label is defined
#32148
prometheusremotewrite: Don't generate target_info
unless at least one identifying label is defined
#32148
Conversation
80658d3
to
9ed9b63
Compare
target_info
unless job is definedtarget_info
unless job
label is defined
cff5f2b
to
19345f6
Compare
May I ask which receiver you are using? OTel SDKs default We already have a config knob for disabling target_info. Does that not meet your needs? |
@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
I'm not looking to disable Considering that |
Thanks for the context.
My understanding is that That being said, I support requiring join keys for target_info to make it useful. I could see us doing either:
(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 |
Thanks for the feedback @dashpole! Let me confer with @jpkrohling. |
@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 |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
19345f6
to
e1da396
Compare
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
e1da396
to
7ac5a7c
Compare
target_info
unless job
label is definedtarget_info
unless at least one identifying label is defined
I've revised the PR as per our Slack discussion @dashpole, thanks for all the feedback. |
…-info-required-attrs
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
CI failure looks like a flake. |
…-info-required-attrs Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot ignore existing service names.
…-info-required-attrs
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
@Aneurysm9 applied your suggestions, PTAL :) |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
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. eitherjob
orinstance
(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 ofprometheusremotewriteexporter
tests that are affected by this change, as they don't define the identifying resource attributes.Documentation: