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

Implement stable database semantic conventions #11575

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

Conversation

heyams
Copy link
Contributor

@heyams heyams commented Jun 13, 2024

@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Jun 13, 2024
@heyams heyams requested a review from a team August 30, 2024 16:24
@heyams
Copy link
Contributor Author

heyams commented Sep 13, 2024

I will add 'error.type' and the opt-in db.query.parameter.<key> in a follow-up PR.

Comment on lines +159 to +163
SemconvStabilityUtil.getAttributeKey(DbIncubatingAttributes.DB_STATEMENT),
"SELECT *"),
entry(
SemconvStabilityUtil.getAttributeKey(DbIncubatingAttributes.DB_OPERATION),
"SELECT"));
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest changing these two lines to reference the stable names directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but we test both non-stable and stable. are you suggest to change gradle config to test stable only?

Comment on lines +200 to +206
SemconvStabilityUtil.getAttributeKey(DbIncubatingAttributes.DB_STATEMENT),
"SELECT * FROM table"),
entry(
SemconvStabilityUtil.getAttributeKey(DbIncubatingAttributes.DB_OPERATION),
"SELECT"),
entry(
SemconvStabilityUtil.getAttributeKey(DbIncubatingAttributes.DB_CASSANDRA_TABLE),
Copy link
Member

Choose a reason for hiding this comment

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

same here

/**
* @deprecated Use {@link #getDbQueryText(REQUEST)} instead.
*/
@Deprecated
@Nullable
String getStatement(REQUEST request);
Copy link
Member

Choose a reason for hiding this comment

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

since this was just a rename, I think could add default implementation that delegates to getDbQueryText, then wouldn't need to override this everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's better to use @deprecate so that when old semconv is removed, we can use it to clean it up.

/**
* @deprecated Use {@link #getOperationName(REQUEST)} instead.
*/
@Deprecated
@Nullable
String getOperation(REQUEST request);
Copy link
Member

Choose a reason for hiding this comment

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

similar here could add default method calling getDbOperationName

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.

2 participants