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

Revert "Add support for OpenTelemetry in JDBC connectors" #19010

Closed
wants to merge 1 commit into from

Conversation

hashhar
Copy link
Member

@hashhar hashhar commented Sep 12, 2023

This reverts commit 7b269fa.

We've observed NullPointerException being thrown like:

Error listing schemas for catalog bootcamppostgresql: java.lang.NullPointerException: Cannot invoke "java.sql.Connection.getMetaData()" because "connection" is null.

The error comes from OpenTelemetry code
https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/9cf710f17c028d017ae0520d595ef72b9917e241/instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcUtils.java#L102.

After a proper fix is available the reverted commit can be reintroduced.

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# XXX
* xxx. ({issue}`issuenumber`)

This reverts commit 7b269fa.

We've observed NullPointerException being thrown like:

    Error listing schemas for catalog bootcamppostgresql: java.lang.NullPointerException: Cannot invoke "java.sql.Connection.getMetaData()" because "connection" is null.

The error comes from OpenTelemetry code
https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/9cf710f17c028d017ae0520d595ef72b9917e241/instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcUtils.java#L102.

After a proper fix is available the reverted commit can be
reintroduced.
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

I think the release entry isn't required because it's not user-facing. At least, we didn't mention the JDBC connector's tracing in the past release notes like other modules.

@martint
Copy link
Member

martint commented Sep 12, 2023

What’s involved in fixing the actual problem? Can we just do that instead of reverting the change?

@hashhar
Copy link
Member Author

hashhar commented Sep 12, 2023

@martint right now I've been unable to find a source for why otel gets null connections.

Yuya submitted a patch to otel to fix the NPE however we need to wait for releases.

I'd prefer to reintroduce this once we have a fix instead of keeping around functionality that we know to be broken.

@martint
Copy link
Member

martint commented Sep 12, 2023

@ebyhr, BTW, if this fixes a user-visible error, it should have a release note. Something like "Fix failure when xxx". It's not about whether the issue is related to tracing or not, but whether the users are visibly affected by the problem. We want to make it easier for people to be able to find out whether a given release fixes a problem they are experiencing.

@electrum
Copy link
Member

Can we simply disable this rather than reverting the entire change? It's best to eliminate code churn when possible.

@hashhar hashhar closed this Sep 13, 2023
@hashhar hashhar deleted the hashhar/revert-otel-jdbc branch September 13, 2023 04:50
@hashhar
Copy link
Member Author

hashhar commented Sep 13, 2023

superseded by #19021

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

Successfully merging this pull request may close these issues.

5 participants