Prometheus Receiver: config to include instance and job labels #2683
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2683 +/- ##
==========================================
- Coverage 92.06% 92.04% -0.03%
==========================================
Files 313 270 -43
Lines 15439 15431 -8
==========================================
- Hits 14214 14203 -11
- Misses 817 847 +30
+ Partials 408 381 -27
Continue to review full report at Codecov.
|
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
A quick question, shouldn't we always do this and not put it under a configuration setting to behave the same way the Prometheus behaves? |
@rakyll I am good with making it always do this to match Prometheus; then the change would just be the two lines in metricbuilder. I just included the config setting in case we wanted to preserve how it works currently. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Please resolve the conflicts. |
|
@tigrannajaryan I have resolved the conflicts |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
@gracewehner is this PR blocked on review? |
|
@Aneurysm9 @rakyll please review @gracewehner please rebase |
| ```yaml | ||
| receivers: | ||
| prometheus: | ||
| include_resource_labels: true |
There was a problem hiding this comment.
I'd not call "instance" and "job" resource labels which sounds like resource attributes which represents something else in Otel.
Also we should have these labels by default to make the Prometheus export work. I would add an option to exclude them.
Why not something like ignore_scrape_labels?
There was a problem hiding this comment.
I'd second this. I think the implementation looks sound, but it would be good to avoid potential confusion of these labels with OTel resource attributes. I'd also think that they should be included by default and a name like ignore_scrape_labels makes it clear that the user is choosing to disable something otherwise useful.
There was a problem hiding this comment.
At the time of this PR, both instance & job did not come from receiver. Now that they are, should we just not do anything here (meaning just close this PR) ?
There was a problem hiding this comment.
@rakyll @Aneurysm9 Do you have an opinion about Vishwa's above comment about how the instance and job labels are now resource attributes (at the original time of this PR they were not)? Are we good with just having them as resource attributes or do we still want them as actual labels, too, which this PR would do? If so, I can make the changes with ignore_scrape_labels suggested above.
There was a problem hiding this comment.
I think this PR can be closed as the job and instance labels are always included via #2897. This PR would have the advantage of exposing configuration to disable inclusion of those labels, but they are required for Prometheus remote write compatibility and giving the user more knobs to tweak isn't always ideal.
|
Closing since job and instance labels are included as resource attributes via #2897 |
Bumps golang from 1.20.1-bullseye to 1.20.2-bullseye. --- updated-dependencies: - dependency-name: golang dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Description:
Adds a config to choose to include the labels
instanceandjobfor metrics scraped by the Prometheus Receiver. Prometheus adds these labels to every metric but they are currently dropped in the receiver.Link to tracking Issue:
Addresses the labels aspect of issue #2363
Testing:
Adds to the test Test_isUsefulLabel to check results when config is set to false vs. true