[Help needed] Deploying opentelemetry-sqlcommenter-java to maven central about opentelemetry-java-contrib

[anuraaga]

I took a closer look through the code. I found a bit of context propagation logic that overlaps with our existing instrumentation and then a very small amount of code to append it to an sql string.

https://github.com/google/sqlcommenter/blob/master/java/sqlcommenter-java/src/main/java/com/google/cloud/sqlcommenter/threadlocalstorage/State.java#L109

I don't see much point in a separate repo or even a folder in this repo given how small this is. How about we just add the SQL injection behind a flag in our existing instrumentation? Work in the spec to define the formatting could happen in parallel with this if it's flag guarded.

[sjs994]

Thanks @anuraaga, @trask. We will close this ticket.
We will try out some POCs to integrate sqlcommenter into opentelemetry-java-instrumentation following the approaches provided by @anuraaga.

[austinlparker]

Hey @sjs994, any update on this?

[trask]

hi @sjs994! what do you think about splitting the sqlcommenter components across the various language contrib repos? that would be the easiest way to benefit from existing / shared build infrastructure and language expertise

[sjs994]

Hi @trask,

Thanks for the reply!

To give more context: open-telemetry/community#783 (comment). This is what we are trying to achieve. As of now we are planning to have it as part of a separate monorepo project and publish packages from here. This will help us deprecate the google-cloud repository: https://github.com/google/sqlcommenter.

After an initial version release, we are planning to start discussion on merging each language based library to the proper opentelemetry-contrib/instrumentation repositories.

Does that make sense ?

[anuraaga]

Hi @sjs994 - I think we won't be very comfortable distributing out publishing credentials across various repositories because it will become quite hard for us to track. The reality of package managers is they still generally require personal credentials for publishing to. I suspect other language maintainers will have a similar sentiment.

From my reading of the google repo, it seems the implementations are all independent. Even if putting the java folder in this repo, your team can still have approval rights to it while also making sure all the OTel Java components are under the technical stewardship of the Java maintainers.

Would you be blocked from deprecating your google repo even if the four implementations were distributed among the OTel language teams?

[sjs994]

Hi @anuraaga,

I agree, getting a token with full scope to deploy changes to any repository is not acceptable.
But from the discussion with Python SIG, we will be getting a new token with scope limited to only sqlcommenter project. That way, we will have the organization/account for PyPi as opentelemetry, but the token scope will be limited to only sqlcommenter.
For maven, my understanding was that we can create users with scope limited to a single project/package and that user's token can be used to publish changes. For instance my nexus account is linked only with google-cloud-sqlcommenter project. So, i can deploy only sqlcommenter changes in google repositories.

We will be unblocked when we will have some versions published from opentelemetry org.

But have a few queries regarding putting java-sqlcommenter folder in contrib repo:

• Can we move the java-sqlcommenter folder as such into contrib repos or would it require some major changes, Would it require any change in specs ?
• Can the libraries be published independently or it would have to wait for the release process of the contrib repo as a whole ? (Reason is this library is used by customers and may require hotfix in case of issues)
• My understanding was that contrib libraries are not production ready and google-sqlcommenter library is used by customers. So, when we deprecate that, this library would be used by customers.

[trask]

hey @sjs994,

Can we move the java-sqlcommenter folder as such into contrib repos or would it require some major changes, Would it require any change in specs ?

moving into the contrib repo doesn't trigger any special spec requirements (maybe I misunderstand this question?). it does require following this repo's conventions (which we need to document better, but we will help you through these)

Can the libraries be published independently or it would have to wait for the release process of the contrib repo as a whole ? (Reason is this library is used by customers and may require hotfix in case of issues)

the release process applies to the whole repo for simplicity (one set of tags across all components), but we can kick off (repo-wide) patch releases as needed. see the patch release policy in the java-instrumentation repo, I will update the docs in this repo to explicitly mention the same (other components in this repo are also used by customers and have similar needs)

My understanding was that contrib libraries are not production ready

contrib repos support both production ready and experimental components

[anuraaga]

@sjs994 I discussed with the other Java maintainers and there is consensus that we will not accept publishing Java artifacts outside of our maintained three repos. That gives two options

1. RECOMMENDED: Add sqlcommenter functionality into our existing database instrumentation behind an experimental flag and work on getting it spec'd out. I have filed open-telemetry/opentelemetry-specification#2279 to get some momentum on the spec side, but spec does not have to block an experimental implementation.

2. Move the existing sqlcommenter-java instrumentation into this contrib repo. I suspect there will be some significant work on getting patterns to match the patterns of OpenTelemetry Java (e.g., don't use String.format, use junit5 + assertj for tests, etc) and wholy suspect this to take more time / coding than 1). We also can't expect the exact same functionality - for example I notice the current instrumentation has a fallback for OpenCensus even though we would expect usage of the opencensus-shim for users of our artifacts and probably would want census-specific code in an OpenTelemetry-provided instrumentation library. A folder in this repo would probably not be a drop-in replacement of the current google artifact, naturally because we are targeting use cases for OpenTelemetry users here, not just taking ownership of vendor code as-is.

Judging from open-telemetry/opentelemetry-python-contrib#845 (comment) on the Python issue, it seems like the primary motivation here is to deprecate the google/sqlcommenter repository. I would flip this expectation around and think again - probably what you should be working for is to deprecate open-telemetry/sqlcommenter. It might sound weird, but that repo was really just meant to be an interim while figuring out how to integrate sqlcommenter into OpenTelemetry - IMO the approach is 1) above. google/sqlcommenter can continue to be maintained until the OTel feature stabilizes, at which point you'd probably provide a document on how to migrate to OpenTelemetry (it likely won't be drop-in). Perhaps you would put the repo into maintenance mode and deprecate it at some point after that.

[anuraaga]

Hi @sjs994 - @jsuereth has summed up his thoughts on incorporating sqlcommenter into instrumentation in open-telemetry/community#783 (comment)

I am excited especially by the 2) there of getting tracing-through-SQL spec'd out and formulated to an open standard.

For Java, I do still strongly recommend adding sqlcommenter functionality to our existing instrumentation, I suspect it will be relatively easy for all of us and the quickest/simplest way to get this into the hands of OTel users. We have JDBC instrumentation which will work in almost all apps, with a single interception point here

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/OpenTelemetryStatement.java#L275

We can tweak the method signature to accept a ThrowingCallable instead of Supplier, passing in sql to the callbacks. Then we should be able to retrieve SpanContext and modify the SQL with it's content

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/jdbc/library/src/main/java/io/opentelemetry/instrumentation/jdbc/internal/OpenTelemetryStatement.java#L277

For an initial version I recommend just dealing with traceparent and tracestate and think about controller/action/framework later. During the specification process it will be good to understand how this interacts with OpenTelemetry baggage or the semantic convention attributes for example.

While I don't think the JDBC instrumentation currently references any configuration, you can see this example, you just need to statically call to retrieve a flag value (jdbc instrumentation is itself statically initialized which makes things simpler for us here). If we guard SQL mutation using Context behind otel.experimental.jdbc.inject-sql.enabled then there is no problem having the code in our instrumentation and it will be quite easy to see how it does in production - notably, both library instrumentation and javaagent users will be able to try it out, the latter being the more common way of using OpenTelemetry in Java as of now I believe.

Does this provide enough info to get started in our repo? Let us know if anything is unclear or you are still blocked on anything.

PS: When bringing the injection logic in, you will probably refer to the existing code. Please make sure to remove String.format and map allocations, you should be able to write all the contents out directly to a StringBuilder.

[sjs994]

Thank you very much @anuraaga!

As now we have clarity, we can go forward on trying to integrate things into opentelemetry. Thanks for the code points where to integrate. I think this provides enough info to start.

We are looking at opentelemetry-python first and have some progress with POCs there. After a document for that with few possible approaches is complete, we will start on the java side.

1. We will create some POCs along with a small document on integration.
2. After that, we can have a discussion in the SIG meeting and go ahead.

Does that work ?

I do have few queries:

1. I suppose we will have a flag to control the experimental feature initially ?
2. Do we need to (take care of/start looking at) auto-instrumentation from start ?

[sjs994]

I suppose we will have a flag to control the experimental feature initially ?

I suppose this flag: otel.experimental.jdbc.inject-sql.enabled is suppose to control the experimental feature ?

[anuraaga]

Yup until the spec is complete we would need a flag like that.

Do we need to (take care of/start looking at) auto-instrumentation from start ?

In Java I think so since many if not most users use it. The instrumentation is the same for both so this shouldn't be something to worry about though. I suspect it is the same situation in Python.

[sjs994]

Thanks, Got it!