Skip to content

Conversation

@SylvainJuge
Copy link
Contributor

Part of #13031 for jdbc instrumentation

@SylvainJuge SylvainJuge self-assigned this Sep 23, 2025
@SylvainJuge SylvainJuge marked this pull request as ready for review September 23, 2025 15:15
@SylvainJuge SylvainJuge requested a review from a team as a code owner September 23, 2025 15:15
Comment on lines 66 to 75
DbRequest request;
if (sql != null) {
request = DbRequest.create(statement, sql);
} else if (preparedStatement != null) {
Map<String, String> parameters = JdbcData.getParameters(preparedStatement);
request = DbRequest.create(preparedStatement, parameters);
} else {
// batch
request = createBatchRequest(statement);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps it would be better for start to accept Supplier<DbRequest> so that the request creation could be moved to startStatement etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to do that, but I see two potential issues:

  • extra allocation due to having to create an supplier instance on each call
  • we might not be able to use lambdas unless the instrumented bytecode is java 8 or later

We could however maybe use an enum rather than an heuristic based on null arguments to split the 3 types of calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

extra allocation due to having to create an supplier instance on each call

This isn't really an issue because we need to create a bunch of objects for each span. I guess you could try you could try subclassing the JdbcAdviceScope If you wish to avoid the supplier or use a stateless lambda, but probably not worth the effort.

we might not be able to use lambdas unless the instrumented bytecode is java 8 or later

that is not an issue since the code is inside JdbcAdviceScope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of pushing the Supplier creation directly into the advice, but you are right keeping it JdbcAdviceScope already makes things easier to read. Once everything is running with indy advices we could even directly remove those 3 intermediate methods that are called from advices.

@laurit laurit merged commit dccbb93 into open-telemetry:main Sep 25, 2025
89 checks passed
@SylvainJuge SylvainJuge deleted the indy-jdbc branch September 25, 2025 13:54
mznet pushed a commit to mznet/opentelemetry-java-instrumentation that referenced this pull request Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants