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

Instrument jdbc batch queries #12797

Merged
merged 12 commits into from
Jan 16, 2025
Merged

Instrument jdbc batch queries #12797

merged 12 commits into from
Jan 16, 2025

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Nov 26, 2024

Part of #12608
Resolves #4476 Note that this issue proposes an attribute (db.statement.error.count) for tracking failed statements in the batch which I didn't notice in the semantic conventions.

@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Nov 26, 2024
@laurit laurit marked this pull request as ready for review November 26, 2024 14:04
@laurit laurit requested a review from a team as a code owner November 26, 2024 14:04
@trask
Copy link
Member

trask commented Dec 6, 2024

Note that this issue proposes an attribute (db.statement.error.count) for tracking failed statements in the batch

I didn't see this in the code, did you end up removing it?

@laurit
Copy link
Contributor Author

laurit commented Dec 6, 2024

Note that this issue proposes an attribute (db.statement.error.count) for tracking failed statements in the batch

I didn't see this in the code, did you end up removing it?

didn't implement it as I couldn't find it in the semconv

@trask
Copy link
Member

trask commented Dec 6, 2024

oh, got it, the issue proposed the attribute, thanks 👍

@trask trask added this to the v2.11.0 milestone Dec 7, 2024
@trask trask modified the milestones: v2.11.0, v2.12.0 Dec 23, 2024
@trask
Copy link
Member

trask commented Jan 16, 2025

@laurit let me know what you think: laurit#408

I'm thinking it's ok to support batches directly in SqlClientAttributesGetter (it will force people to think about it).

The other changes are mainly me re-writing some of the extractor code in the process of trying to make it make sense to me. I'm not sure if I ended up just replacing your complex code with my complex code though 😅.

@laurit
Copy link
Contributor Author

laurit commented Jan 16, 2025

@laurit let me know what you think: laurit#408

I'm thinking it's ok to support batches directly in SqlClientAttributesGetter (it will force people to think about it).

merged. I also considered this but wasn't sure we want to force the multiple statement handling on all sql instrumentations since jdbc seemed to be the only one that uses it.

The other changes are mainly me re-writing some of the extractor code in the process of trying to make it make sense to me. I'm not sure if I ended up just replacing your complex code with my complex code though 😅.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

thanks @laurit!

@trask trask merged commit 467dfc0 into open-telemetry:main Jan 16, 2025
58 checks passed
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.

Instrument java.sql.Statement.executeBatch
2 participants