Skip to content

Db client error type for JDBC #13331

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 28 commits into from
Mar 31, 2025
Merged

Db client error type for JDBC #13331

merged 28 commits into from
Mar 31, 2025

Conversation

@zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Feb 17, 2025

Part of #12804

@zeitlinger zeitlinger requested a review from a team as a code owner February 17, 2025 15:22
@zeitlinger zeitlinger self-assigned this Feb 17, 2025
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Feb 17, 2025
@zeitlinger zeitlinger changed the title Db client error type Db client error type for JDBC Feb 17, 2025
@zeitlinger zeitlinger force-pushed the db-client-error-type branch from dfd54d9 to fe0ff1c Compare March 11, 2025 15:20
@zeitlinger
Copy link
Member Author

zeitlinger commented Mar 12, 2025

@laurit addressed/answered everything - can you check again?

@zeitlinger zeitlinger force-pushed the db-client-error-type branch from 38a95f0 to d09a0ae Compare March 13, 2025 12:18
@zeitlinger zeitlinger added this to the v2.14.0 milestone Mar 13, 2025
@zeitlinger zeitlinger force-pushed the db-client-error-type branch from 0cf2546 to ae2e7a7 Compare March 13, 2025 15:41
@trask trask removed this from the v2.14.0 milestone Mar 13, 2025
@trask
Copy link
Member

trask commented Mar 13, 2025

@zeitlinger can you merge in main to fix the CI failures? thanks

@zeitlinger zeitlinger force-pushed the db-client-error-type branch from ae2e7a7 to 31f9b30 Compare March 13, 2025 18:39
@zeitlinger
Copy link
Member Author

zeitlinger commented Mar 13, 2025

@zeitlinger can you merge in main to fix the CI failures? thanks

done

@zeitlinger zeitlinger force-pushed the db-client-error-type branch from d46e639 to b0f00f7 Compare March 17, 2025 10:49
@zeitlinger
Copy link
Member Author

zeitlinger commented Mar 18, 2025

@trask works

trask
trask approved these changes Mar 26, 2025
View reviewed changes
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!

zeitlinger and others added 21 commits March 31, 2025 10:37
@zeitlinger
error.type and db.response.status_code for jdbc
dc2d3a1
@zeitlinger
error.type and db.response.status_code all dbs
5c98aa5
@zeitlinger
error.type and db.response.status_code all dbs
6de35ee
@zeitlinger
error.type and db.response.status_code all dbs
772e038
@zeitlinger
error.type and db.response.status_code all dbs
49fc4b1
@zeitlinger
error.type and db.response.status_code all dbs
d1ec77c
@zeitlinger
error.type and db.response.status_code all dbs
fe22eb9
@zeitlinger
error.type and db.response.status_code all dbs
c027b8e
@zeitlinger
error.type and db.response.status_code all dbs
2b68ae2
@zeitlinger
error.type and db.response.status_code all dbs
28448df
@zeitlinger
error.type and db.response.status_code all dbs
9cbff11
@zeitlinger
format
23d0b7f
@zeitlinger
use single method for status code
9489d9b
@zeitlinger
use single method for status code
64c3177
@zeitlinger
only for stable semconv
0be0c62
@zeitlinger
only for stable semconv
8743e05
@zeitlinger
only for stable semconv
537795e
@zeitlinger
only for stable semconv
2081dfa
@zeitlinger @jaydeluca
Update instrumentation/jdbc/library/src/test/java/io/opentelemetry/in…
854974e 
…strumentation/jdbc/datasource/JdbcTelemetryTest.java

Co-authored-by: Jay DeLuca <[email protected]>
@zeitlinger
format
fa2d573
@zeitlinger
pr review
5cf76cb
@zeitlinger zeitlinger force-pushed the db-client-error-type branch from b0f00f7 to 5cf76cb Compare March 31, 2025 09:04
@zeitlinger
Copy link
Member Author

zeitlinger commented Mar 31, 2025

@trask all done 😄

@trask trask merged commit e2cce4c into open-telemetry:main Mar 31, 2025
87 checks passed
@zeitlinger zeitlinger deleted the db-client-error-type branch March 31, 2025 12:45
laurit
laurit reviewed Apr 1, 2025
View reviewed changes
...instrumentation/elasticsearch/rest/common/v5_0/internal/ElasticsearchDbAttributesGetter.java
public String getResponseStatus(@Nullable Response response, @Nullable Throwable error) {
if (response != null) {
int httpStatus = response.getStatusLine().getStatusCode();
return httpStatus >= 400 && httpStatus < 600 ? Integer.toString(httpStatus) : null;
Copy link
Contributor

@laurit laurit Apr 1, 2025

Choose a reason for hiding this comment

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

I guess the intent here is to only use http status codes that are errors. For the same purpose we use slightly different bounds in

opentelemetry-java-instrumentation/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpStatusCodeConverter.java

Lines 19 to 27 in 81f3795

CLIENT {
@Override
boolean isError(int responseStatusCode) {
return responseStatusCode >= 400
||
// invalid status code, does not exists
responseStatusCode < 100;
}
};

Copy link
Member Author

@zeitlinger zeitlinger Apr 1, 2025

Choose a reason for hiding this comment

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

so should we move this to some internal package to promote consistency?

Copy link
Contributor

@laurit laurit Apr 1, 2025

Choose a reason for hiding this comment

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

I guess it could be extracted into a separate util that could also be used from the db instrumentations. Alternatively we could start from having an internal util class that handles the status code for the db instrumentations that use http statuses that behave the same as this code (providing this behavior make sense for the db instrumentations).

laurit
laurit reviewed Apr 1, 2025
View reviewed changes
.../src/test/java/io/opentelemetry/javaagent/instrumentation/spymemcached/SpymemcachedTest.java
Comment on lines +899 to +904
withErrorType(
"java.lang.IllegalArgumentException",
equalTo(
maybeStable(DB_SYSTEM),
DbIncubatingAttributes.DbSystemIncubatingValues.MEMCACHED),
equalTo(maybeStable(DB_OPERATION), "decr")))));
Copy link
Contributor

@laurit laurit Apr 1, 2025

Choose a reason for hiding this comment

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

Another way this cold be approached is using equalTo(ERROR_TYPE, SemconvStability.emitStableDatabaseSemconv() ? "java.lang.IllegalArgumentException" : null)

laurit
laurit reviewed Apr 1, 2025
View reviewed changes
...o/opentelemetry/javaagent/instrumentation/vertx/v4_0/sql/VertxSqlClientAttributesGetter.java
public String getResponseStatus(@Nullable Void response, @Nullable Throwable error) {
try {
// loaded via reflection, because this class is not available in all versions that we support
Class<?> ex = Class.forName("io.vertx.pgclient.PgException");
Copy link
Contributor

@laurit laurit Apr 1, 2025

Choose a reason for hiding this comment

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

Usually we'd do the Class.forName and getMethod only once and cache the result. Secondly postgres is not the only database supported by vert.x where is the handling for other dbs? It seems that at some point they introduced a common DatabaseException base class, imo you should implement support for at least that.

Copy link
Member Author

@zeitlinger zeitlinger Apr 2, 2025

Choose a reason for hiding this comment

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

just adding that.

BTW, one extents the other exception

image

Copy link
Contributor

@laurit laurit Apr 2, 2025

Choose a reason for hiding this comment

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

BTW, one extents the other exception

DatabaseException was introduced in 4.4.0, we support 4.0.0

Copy link
Member Author

@zeitlinger zeitlinger Apr 2, 2025

Choose a reason for hiding this comment

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

The postgres exception extents Database exception - in later versions of the lib

@zeitlinger zeitlinger mentioned this pull request Apr 2, 2025
Merged
@zeitlinger
Copy link
Member Author

zeitlinger commented Apr 2, 2025

@laurit I've created #13640 as a follow up - please add comments there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laurit laurit laurit left review comments

@trask trask trask approved these changes

@jaydeluca jaydeluca jaydeluca approved these changes

Assignees

@zeitlinger zeitlinger

Labels

test native This label can be applied to PRs to trigger them to run native tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zeitlinger @trask @laurit @jaydeluca