search

jenkinsci / junit-sql-storage-plugin

https://plugins.jenkins.io/junit-sql-storage/
MIT License
6 stars 14 forks source link

Instrument jdbc datasource to see SQL statement spans in the traces #416

Closed cyrille-leclerc closed 2 months ago

cyrille-leclerc commented 2 months ago

Instrument the JDBC data source with OpenTelemetry to see SQL statement spans in the traces.

Testing done

Integration test done:

image

Submitter checklist

timja commented 2 months ago

I was able to make it work by using the unwrapped datasource for DB migration, that could be a solution but a better solution would probably be to set this to a somewhat high number so otel is configured before most other things:

https://github.com/jenkinsci/opentelemetry-plugin/blob/fa8285748dbd836c374662b20447e20e1ae4f468/src/main/java/io/jenkins/plugins/opentelemetry/OpenTelemetrySdkProvider.java#L45

From a working one this is the log order:

2024-06-18 07:11:39.538+0000 [id=45]    INFO    jenkins.InitReactorRunner$1#onAttained: System config adapted
2024-06-18 07:11:39.729+0000 [id=49]    INFO    o.f.c.i.logging.slf4j.Slf4jLog#info: Flyway Community Edition 9.22.3 by Redgate
2024-06-18 07:11:39.729+0000 [id=49]    INFO    o.f.c.i.logging.slf4j.Slf4jLog#info: See release notes here: https://rd.gt/416ObMi
2024-06-18 07:11:39.729+0000 [id=49]    INFO    o.f.c.i.logging.slf4j.Slf4jLog#info: 
2024-06-18 07:11:39.739+0000 [id=49]    INFO    o.f.c.i.logging.slf4j.Slf4jLog#info: Database: jdbc:postgresql://localhost/postgres (PostgreSQL 13.4)
2024-06-18 07:11:39.769+0000 [id=49]    INFO    o.f.c.i.logging.slf4j.Slf4jLog#info: Successfully validated 5 migrations (execution time 00:00.018s)
2024-06-18 07:11:39.796+0000 [id=49]    INFO    o.f.c.i.logging.slf4j.Slf4jLog#info: Current version of schema "public": 2020.12.15.1153
2024-06-18 07:11:39.797+0000 [id=49]    INFO    o.f.c.i.logging.slf4j.Slf4jLog#info: Schema "public" is up to date. No migration necessary.
2024-06-18 07:11:41.835+0000 [id=45]    INFO    i.j.p.o.OpenTelemetrySdkProvider#initializeOtlp: OpenTelemetry SDK initialized: SDK [config: otel.traces.exporter=otlp, otel.metrics.exporter=otlp, otel.exporter.otlp.endpoint=http://localhost:4317, resource: service.name=jenkins, service.namespace=jenkins, service.version=2.462]
2024-06-18 07:11:41.871+0000 [id=45]    INFO    i.j.p.o.i.ServletFilterInitializer#afterSdkInitialized: Jenkins Remote Span disabled
2024-06-18 07:11:41.893+0000 [id=45]    INFO    jenkins.InitReactorRunner$1#onAttained: Loaded all jobs

I've tested it with this PR and the PR that links it to a Run: https://github.com/jenkinsci/junit-sql-storage-plugin/pull/414 Both work great, thanks!

Just let me know what you'd prefer, disabling telemetry for DB Migration or fixing this in the otel plugin, and also switch this to the API plugin

cyrille-leclerc commented 2 months ago

Thanks @timja, I'm sorry I didn't catch the problem you encountered. I propose that we first cleanup the dependencies between plugins adopting https://github.com/jenkinsci/opentelemetry-api-plugin

Then we reevaluate the problem you identified above as it looks like a class loading / plugin loading problem.

Would that make sense to you?

timja commented 2 months ago

Sure

timja commented 2 months ago

Thanks @timja, I'm sorry I didn't catch the problem you encountered. I propose that we first cleanup the dependencies between plugins adopting jenkinsci/opentelemetry-api-plugin

Then we reevaluate the problem you identified above as it looks like a class loading / plugin loading problem.

Would that make sense to you?

I still get the error:

Caused: java.lang.IllegalStateException: GlobalOpenTelemetry.set has already been called. GlobalOpenTelemetry.set must be called only once before any calls to GlobalOpenTelemetry.get. If you are using the OpenTelemetrySdk, use OpenTelemetrySdkBuilder.buildAndRegisterGlobal instead. Previous invocation set to cause of this exception.

cyrille-leclerc commented 2 months ago

Problem understood: io.jenkins.plugins.junit.storage.database.DatabaseSchemaLoader#migrateSchema() is invoked very early thanks to @Initializer(after = SYSTEM_CONFIG_ADAPTED), before the Jenkins OTel Plugin is initialized and thus defined GlobalOpenTelemetry.set().

Interestingly, the Otel Plugin also tries to initialize very early, but get initialized after @Initializer(after = SYSTEM_CONFIG_ADAPTED) DatabaseSchemaLoader#migrateSchema() is invoked.

The Otel Plugin init directive: @Initializer(after = InitMilestone.SYSTEM_CONFIG_ADAPTED, before = InitMilestone.JOB_LOADED) initializeOpenTelemetry()

-

https://github.com/jenkinsci/junit-sql-storage-plugin/blob/90e2aaaa0dd70fcd3aed93548294c2b9487bc5d3/src/main/java/io/jenkins/plugins/junit/storage/database/DatabaseSchemaLoader.java#L24-L25

-

https://github.com/jenkinsci/opentelemetry-plugin/blob/61564d5fb05f67972db57b607901eb91481da55e/src/main/java/io/jenkins/plugins/opentelemetry/JenkinsOpenTelemetryPluginConfiguration.java#L221-L228

-

FYI the opentelemetry-api-plugin may not work (see critical bug here).

cyrille-leclerc commented 2 months ago

I put this PR as draft because we need to work on the Jenkins OTel Plugin to either:

timja commented 2 months ago

I put this PR as draft because we need to work on the Jenkins OTel Plugin to either:

  • initialize the Jenkins OTel Plugin before @Initializer(after = SYSTEM_CONFIG_ADAPTED) DatabaseSchemaLoader#migrateSchema() is invoked
  • Provide, hopefully just as a temporary workaround, an alternative to GlobalOpenTelemetry.get() to ensure the Otel Plugin initializes GlobalOpenTelemetry before other plugins use it through GlobalOpenTelemetry.get()

I think you should just be able to adjust the ordinal of the extension but you'll need to make sure JCasC is invoked first.

timja commented 2 months ago

Ah JCasc should be fine as its done in the before: https://github.com/jenkinsci/configuration-as-code-plugin/blob/3b0e6de4ab83e8024e6cac635728c28b68ac663a/plugin/src/main/java/io/jenkins/plugins/casc/ConfigurationAsCode.java#L337

cyrille-leclerc commented 2 months ago

No worries, I think I found the solution in the Jenkins OTel Plugin to initialize GlobalOpenTelemetry.get() very early

I think you should just be able to adjust the ordinal of the extension but you'll need to make sure JCasC is invoked first.

Do you have details on this? I'm not familiar with this concept.

timja commented 2 months ago

Do you have details on this? I'm not familiar with this concept.

https://github.com/jenkinsci/jenkins/blob/c674d94e29784ea35973fd11e3ed8f8aa0888356/core/src/main/java/hudson/Extension.java#L85

I'm not sure if its supported in initialisers I haven't found out either way from a bit of research

cyrille-leclerc commented 2 months ago

will. be solved in the Jenkins Database Plugin