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

reuse semconv incubating attributes in test code #12992

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

SylvainJuge
Copy link
Contributor

Incubating semconv attributes should not be used in production code to prevent introducing compatibility issues.

However in test code the semconv attributes duplication is harmful as it makes maintenance more tedious, so this PR removes duplication for some attributes. This is not meant to be an exhaustive cleanup so leftovers are expected.

@SylvainJuge SylvainJuge requested a review from a team as a code owner January 3, 2025 16:43
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Jan 6, 2025
entry(AttributeKey.stringKey("db.namespace"), "potatoes"),
entry(AttributeKey.stringKey("db.query.text"), "SELECT * FROM potato"),
entry(AttributeKey.stringKey("db.operation.name"), "SELECT"));
entry(DbIncubatingAttributes.DB_NAMESPACE, "potatoes"),
Copy link
Member

Choose a reason for hiding this comment

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

is there any reason not to use static imports for all of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only to minimize the diff and keep things consistent, do you think we should migrate and use static imports like in some other files ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could skip static imports in this file to reduce diff size. In #12920 @trask used a script to automatically add the static imports, but that script only ran under instrumentation. We could run that script in instrumentation-api-incubator in a separate PR. Use static imports in the changes that are under instrumentation, all (or at least most) of the usages should already use static imports there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test native This label can be applied to PRs to trigger them to run native tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants