Skip to content

Configure to promote all resource attributes except ignore ones using 'otlp.ignore_resource_attributes' #16426

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
May 26, 2025
Merged

Configure to promote all resource attributes except ignore ones using 'otlp.ignore_resource_attributes' #16426

merged 14 commits into from
May 26, 2025

Conversation

antonjim-te
Copy link
Contributor

Fixes #15158

Configure to promote all resource attributes except ignore ones using new config field 'otlp.ignore_resource_attributes'

It cannot be configured simultaneously with 'promote_resource_attributes'. Default stay as it is today to not promote any resource attribute.

You can now promote all resource attributes by setting otlp.ignore_resource_attributes: []

@antonjim-te antonjim-te force-pushed the resource_attributes branch from a325e2b to 39be55f Compare April 13, 2025 10:14
@antonjim-te antonjim-te mentioned this pull request Apr 13, 2025
Closed
jmichalek132
jmichalek132 reviewed Apr 14, 2025
View reviewed changes
ArthurSens
ArthurSens reviewed Apr 14, 2025
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks pretty solid! I've added a few comments here and there.

Before approving I'd love to get other maintainers to review the idea itself as well

aknuds1
aknuds1 requested changes Apr 15, 2025
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in my previous comment on the feature request, I suspect that there should still be an explicit configuration parameter for enabling promotion of all resource attributes.

@antonjim-te
Copy link
Contributor Author

As mentioned in my previous comment on the feature request, I suspect that there should still be an explicit configuration parameter for enabling promotion of all resource attributes.

Replied there #15158 (comment)

@antonjim-te antonjim-te force-pushed the resource_attributes branch from ca9bc96 to e27e625 Compare April 16, 2025 06:51
@aknuds1
Copy link
Contributor

aknuds1 commented Apr 17, 2025

Taking a look.

aknuds1
aknuds1 reviewed Apr 17, 2025
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said in my previous review, I think promoting all resource attributes should have an explicit configuration parameter.

@antonjim-te
Configure to promote all resoruce attributes except ignore ones using…
6e6216f 
… 'ignore_resource_attributes'

Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@antonjim-te
Configure to promote all resoruce attributes except ignore ones using…
d8a27cf 
… 'ignore_resource_attributes'

Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@antonjim-te
Run lintter
5dc7fe7 
Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@antonjim-te
Apply feedback
57442a4 
Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@antonjim-te
Apply feedback
f7ade07 
Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@antonjim-te
Add promote all
07bea80 
Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@antonjim-te
Review
37888b7 
Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@antonjim-te antonjim-te force-pushed the resource_attributes branch from 1ed97bc to 37888b7 Compare April 21, 2025 20:05
@antonjim-te antonjim-te requested a review from aknuds1 April 21, 2025 20:05
@ArthurSens
Copy link
Member

Do I understand correctly that ignore_resource_attributes doesn't do anything if promote_all_resource_attributes isn't true? 🤔

@aknuds1
Copy link
Contributor

aknuds1 commented Apr 23, 2025 •
edited
Loading

Do I understand correctly that ignore_resource_attributes doesn't do anything if promote_all_resource_attributes isn't true? 🤔

@ArthurSens I haven't reviewed yet, but that sounds like the logical behaviour to me. Logically, why should otlp.ignore_resource_attributes have an effect if Prometheus is in the standard mode of only promoting resource attributes configured through otlp.promote_resource_attributes (i.e. allowlisting mode)? What makes sense to me is that otlp.promote_all_resource_attributes enables the alternate mode of blocklisting resource attributes which shouldn't be promoted. They are two mutually exclusive modes.

jmichalek132
jmichalek132 approved these changes Apr 23, 2025
View reviewed changes
Copy link
Contributor

@jmichalek132 jmichalek132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@aknuds1 aknuds1 requested a review from Copilot April 24, 2025 07:01
Copilot
Copilot AI reviewed Apr 24, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new method for configuring the promotion of resource attributes using a new configuration field (otlp.ignore_resource_attributes) instead of the legacy promote_resource_attributes slice. The changes include updated handling in the write handler and Prometheus remote write translator, revised configuration validation and documentation updates, as well as expanded test cases to cover both the legacy and new behaviors.

  • Updated write handler to use the new ResourceAttributesSetting instance.
  • Added new types and logic in the Prometheus remote write translator to support promotion of all resource attributes except the ignored ones.
  • Modified configuration validation, documentation, and test cases accordingly.

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
storage/remote/write_handler.go Updated configuration usage from PromoteResourceAttributes to ResourceAttributesSetting.
storage/remote/otlptranslator/prometheusremotewrite/metrics_to_prw.go Introduced ResourceAttributeAction, ResourceAttributesSetting, and related helper functions.
storage/remote/otlptranslator/prometheusremotewrite/helper_test.go Updated test cases to reflect the changes in resource attribute promotion configuration.
storage/remote/otlptranslator/prometheusremotewrite/helper.go Refactored label creation to use the new ResourceAttributesSetting and added a call to reset the setting.
docs/configuration/configuration.md Revised documentation to include new configuration fields and usage guidelines.
config/* Updated configuration structure, validation, and test files to support the new resource attribute promotion logic.

aknuds1
aknuds1 reviewed Apr 24, 2025
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting my review. You'll have to fix the merge conflict at least.

aknuds1
aknuds1 requested changes Apr 24, 2025
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my suggestions for a solution that's both easier to reason about and diverges less from the original code (i.e., reduces the amount of changes).

antonjim-te and others added 2 commits April 27, 2025 13:53
@antonjim-te
Apply PR feedback
9603971 
Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@antonjim-te
Merge branch 'main' into resource_attributes
29cd42c 
Signed-off-by: Antonio Jimenez <123171955+antonjim-te@users.noreply.github.com>
@antonjim-te antonjim-te requested a review from aknuds1 April 27, 2025 11:54
@antonjim-te
Review
5af0a4d 
Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@aknuds1 aknuds1 requested a review from Copilot April 27, 2025 12:27
Copilot
Copilot AI reviewed Apr 27, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors resource attribute promotion by introducing a new PromoteResourceAttributes type and associated logic, allowing users to configure promotion of all resource attributes (with the option to ignore specific ones) while enforcing mutual exclusivity with the previous promote_resource_attributes configuration.

  • Updated the write handler to use a constructor for PromoteResourceAttributes.
  • Adjusted tests and documentation to reflect the new pointer‐based configuration and mutual exclusive validations.

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
storage/remote/write_handler.go Refactored to use the new PromoteResourceAttributes constructor.
storage/remote/otlptranslator/prometheusremotewrite/metrics_to_prw.go Introduced the new PromoteResourceAttributes type and updated settings handling.
storage/remote/otlptranslator/prometheusremotewrite/helper_test.go Updated test cases to work with the pointer-based PromoteResourceAttributes.
storage/remote/otlptranslator/prometheusremotewrite/helper.go Replaced the manual loop with a call to the promotedAttributes method.
docs/configuration/configuration.md Documented the new configuration fields and clarified their mutually exclusive usage.
config/* Updated configuration validation and tests to enforce exclusivity between promotion and ignore settings.

aknuds1
aknuds1 requested changes Apr 27, 2025
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please revert the changes to the existing test cases in storage/remote/otlptranslator/prometheusremotewrite/helper_test.go? The PR should not change pre-existing test cases. Just add the test case fields I suggest, and modify the creation of Settings to use NewPromoteResourceAttributes accordingly instead.

@antonjim-te
Apply PR feedback
9145388 
Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@antonjim-te
Apply PR feedback
b99ae4d 
Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@antonjim-te antonjim-te requested a review from aknuds1 April 29, 2025 06:37
aknuds1
aknuds1 requested changes May 4, 2025
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better now, but some issues still remain - please see suggestions.

@antonjim-te
Apply PR feedback
adeb83d 
Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@antonjim-te antonjim-te requested a review from aknuds1 May 5, 2025 15:22
ArthurSens
ArthurSens approved these changes May 16, 2025
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one small nit

@antonjim-te
Apply PR feedback
4995997 
Signed-off-by: Antonio Jimenez <antonjim@thousandEyes.com>
@antonjim-te
Copy link
Contributor Author

@aknuds1 could you please take a look at the PR? Thanks :)

@cstyan cstyan removed their request for review May 21, 2025 16:14
aknuds1
aknuds1 approved these changes May 26, 2025
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@aknuds1 aknuds1 merged commit 2834a66 into prometheus:main May 26, 2025
27 checks passed
@charleskorn charleskorn mentioned this pull request May 30, 2025
Merged
@heliapb heliapb mentioned this pull request Jul 2, 2025
Merged
5 tasks
@dongjiang1989 dongjiang1989 mentioned this pull request Jul 16, 2025
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@aknuds1 aknuds1 aknuds1 approved these changes

@ArthurSens ArthurSens ArthurSens approved these changes

@jesusvazquez jesusvazquez Awaiting requested review from jesusvazquez jesusvazquez is a code owner

@bwplotka bwplotka Awaiting requested review from bwplotka bwplotka is a code owner

@tomwilkie tomwilkie Awaiting requested review from tomwilkie tomwilkie is a code owner

+1 more reviewer

@jmichalek132 jmichalek132 jmichalek132 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Configure Promoting resource attributes
4 participants
@antonjim-te @aknuds1 @ArthurSens @jmichalek132