Skip to content
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

[receiver_creator] add receiver-specific resource attributes #11766

Conversation

rmfitzpatrick
Copy link
Contributor

Description:
Adding a feature - These changes add a new per-receiver resource_attributes config mapping that allows individual receivers to have additional resource attributes beyond those provided at the global level. They also correctly document the existing global resource_attributes expected format and add endpoint type validation.

Testing: Added additional unit tests.

Documentation: Update readme for new feature and correct example of global resource_attributes form.

@rmfitzpatrick rmfitzpatrick requested review from a team and Aneurysm9 June 27, 2022 19:58
@rmfitzpatrick rmfitzpatrick force-pushed the receivercreatorresourceattributes branch 2 times, most recently from 586974f to 4056dec Compare June 28, 2022 13:43
@rmfitzpatrick rmfitzpatrick force-pushed the receivercreatorresourceattributes branch from 4056dec to 694b6db Compare July 7, 2022 15:56
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jul 22, 2022
@rmfitzpatrick rmfitzpatrick force-pushed the receivercreatorresourceattributes branch from 694b6db to 4ad5d44 Compare July 22, 2022 14:32
@rmfitzpatrick
Copy link
Contributor Author

@Aneurysm9 and @dmitryax, mind taking a look when possible?

@github-actions github-actions bot removed the Stale label Jul 23, 2022
Comment on lines +50 to +53
if expr == "" {
delete(attrs, attr)
continue
}
Copy link
Member

Choose a reason for hiding this comment

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

We have similar functionality where we use nil to delete attributes. Should we probably apply the same approach here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing implementation assumes empty strings aren't desirable for attribute values and these changes are in line with that precedent. I can see how this may be overly opinionated as is but think it would be a breaking change to adopt. If we want it I'd prefer to shift everything over to map[string]*string in a subsequent PR to keep the scope reduced and help with reducing unforeseen side effects.

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with this approach as long as we don't want users to have empty string values

@dmitryax dmitryax added the ready to merge Code review completed; ready to merge by maintainers label Jul 25, 2022
and validate global resource_attributes endpoint type map keys
@rmfitzpatrick rmfitzpatrick force-pushed the receivercreatorresourceattributes branch from 4ad5d44 to e35a7cc Compare July 27, 2022 13:14
@rmfitzpatrick
Copy link
Contributor Author

@mx-psi, mind taking another look when able?

@dmitryax dmitryax merged commit 8505e94 into open-telemetry:main Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants