Refactor the `OTelSdkProvider` into an implementation of `OpenTelemetry` #704

cyrille-leclerc · 2023-09-15T15:24:27Z

Refactor the OTelSdkProvider to make it an implementation of OpenTelemetry that is intended to live on a Jenkins Controller.

Testing done

Submitter checklist

Give feedback

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue
Options

Signed-off-by: Cyrille Le Clerc <[email protected]>

# Conflicts: # src/main/java/io/jenkins/plugins/opentelemetry/OpenTelemetrySdkProvider.java # src/main/java/io/jenkins/plugins/opentelemetry/job/log/OtelLogStorage.java # src/main/java/io/jenkins/plugins/opentelemetry/job/log/OtelLogStorageFactory.java

Signed-off-by: Cyrille Le Clerc <[email protected]>

# Conflicts: # src/main/java/io/jenkins/plugins/opentelemetry/JenkinsOpenTelemetryPluginConfiguration.java # src/main/java/io/jenkins/plugins/opentelemetry/OpenTelemetrySdkProvider.java

…LoggerProvider Signed-off-by: Cyrille Le Clerc <[email protected]>

cyrille-leclerc · 2024-06-20T21:52:25Z

Sorry @kuisathaverat I made some changes to better support OTel instrumentation in other plugins. Could you please review again the PR 👼
Note that I have added many unit tests with the last commit

kuisathaverat · 2024-06-21T08:48:35Z

src/main/java/io/jenkins/plugins/opentelemetry/opentelemetry/InstrumentationScope.java

+public class InstrumentationScope {
+    @Nonnull
+    final String instrumentationScopeName;
+    @Nullable
+    final String schemaUrl;
+    @Nullable
+    final String instrumentationScopeVersion;


I think it is worth adding a JavaDoc description of the fields and the class

kuisathaverat · 2024-06-21T08:49:09Z

...n/java/io/jenkins/plugins/opentelemetry/opentelemetry/ReconfigurableEventLoggerProvider.java

+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+
+public class ReconfigurableEventLoggerProvider implements EventLoggerProvider {


I think it is worth adding a JavaDoc description to the class

kuisathaverat · 2024-06-21T08:49:19Z

src/main/java/io/jenkins/plugins/opentelemetry/opentelemetry/ReconfigurableLoggerProvider.java

+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+
+public class ReconfigurableLoggerProvider implements LoggerProvider {


I think it is worth adding a JavaDoc description to the class

kuisathaverat · 2024-06-21T08:49:31Z

src/main/java/io/jenkins/plugins/opentelemetry/opentelemetry/ReconfigurableOpenTelemetry.java

+/**
+ * Reconfigurable {@link OpenTelemetry}
+ */
+public class ReconfigurableOpenTelemetry implements OpenTelemetry, Closeable {


I think it is worth adding a JavaDoc description to the class

kuisathaverat · 2024-06-21T08:49:45Z

src/main/java/io/jenkins/plugins/opentelemetry/opentelemetry/ReconfigurableTracerProvider.java

+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+
+public class ReconfigurableTracerProvider implements TracerProvider {


I think it is worth adding a JavaDoc description to the class

kuisathaverat

minor comments LGTM

cyrille-leclerc · 2024-06-21T09:32:08Z

Thanks, I'll javadoc in a subsequent PR

timja · 2024-06-21T15:27:09Z

src/main/java/io/jenkins/plugins/opentelemetry/JenkinsOpenTelemetryPluginConfiguration.java

+     * as early as possible in Jenkins lifecycle so any plugin invoking those Global setters will have the
+     * reconfigurable instance .
+     */
+    @Initializer(after = InitMilestone.EXTENSIONS_AUGMENTED, before = InitMilestone.SYSTEM_CONFIG_LOADED)


before system config loaded? how will the configuration be available for this plugin? and how will someone configure this plugin with JCasC if its so early?

jcasc is after system config loaded:
https://github.com/jenkinsci/configuration-as-code-plugin/blob/3b0e6de4ab83e8024e6cac635728c28b68ac663a/plugin/src/main/java/io/jenkins/plugins/casc/ConfigurationAsCode.java#L337

Correct: There reasons is:

we want to be sure that any plugin that is using OTel will use our implementation of OpenTelemetry, even when getting this OpenTelemetry instance through GlobalOpenTelemetry.get()

we don't know how to ensure the OpenTelemetry plugin gets configured & initialized before all other plugins (e.g. the initialization code DatabaseSchemaLoader#migrateSchema() of the JUnit SQL Storage Plugin is invoked by Jenkins before the Jenkins OTel Plugin initialization code JenkinsOpenTelemetryPluginConfiguration#initializeOpenTelemetry() despite the dependency.

The solution we identified is to:

Register a Reconfigurable OpenTelemetry instance in GlobalOpenTelemetry as early as possible in Jenkin's lifecycle with a NoOp implementation (see initializeOpenTelemetryAfterExtensionsAugmented()).

When the Jenkins Otel Plugin config is ready, we reconfigure the OpenTelmetry instance (see initializeOpenTelemetry())

I think we can cleanup a bit this code but it works and it was quite time consuming to get there :-)

Dos it make sense?

https://github.com/jenkinsci/junit-sql-storage-plugin/blob/76cefd5f4cf6117553fa6f219442a9cbba64b458/src/main/java/io/jenkins/plugins/junit/storage/database/DatabaseSchemaLoader.java#L24-L26

ah I see bit confusing but it'll work I think

cyrille-leclerc added 14 commits September 15, 2023 17:14

Cleanup OTelSdkProvider

b3556b2

Signed-off-by: Cyrille Le Clerc <[email protected]>

merge master

2394b80

Cleanup OTelSdkProvider

55a78df

Signed-off-by: Cyrille Le Clerc <[email protected]>

Cleanup OTelSdkProvider

c4e88e8

Signed-off-by: Cyrille Le Clerc <[email protected]>

Merge branch 'master' into cleanup-otel-provider

d350353

Merge branch 'master' into cleanup-otel-provider

16f15ad

merge master branch

db9f6b1

Signed-off-by: Cyrille Le Clerc <[email protected]>

Merge branch 'master' into cleanup-otel-provider

4563cb5

Cleanup OTelSdkProvider

5094187

Signed-off-by: Cyrille Le Clerc <[email protected]>

Cleanup OTelSdkProvider

0fbe955

Signed-off-by: Cyrille Le Clerc <[email protected]>

Merge branch 'master' into cleanup-otel-provider

f80b493

Merge master

4001b38

Signed-off-by: Cyrille Le Clerc <[email protected]>

Merge master

d6ab809

Signed-off-by: Cyrille Le Clerc <[email protected]>

cyrille-leclerc marked this pull request as ready for review January 2, 2024 12:12

cyrille-leclerc requested a review from kuisathaverat as a code owner January 2, 2024 12:12

cyrille-leclerc changed the title ~~Cleanup OTelSdkProvider~~ Refactor the OTelSdkProvider into an implementation of openTelemetry Jan 2, 2024

kuisathaverat previously approved these changes Jan 2, 2024

View reviewed changes

kuisathaverat added the chore label Jan 2, 2024

cyrille-leclerc added 3 commits January 28, 2024 22:47

Merge branch 'main' into cleanup-otel-provider

a67e5cf

Merge branch 'main' into cleanup-otel-provider

03028bd

Merge branch 'main' into cleanup-otel-provider

1c8693d

cyrille-leclerc requested a review from kuisathaverat June 5, 2024 09:01

Merge branch 'main' into cleanup-otel-provider

9def781

cyrille-leclerc marked this pull request as draft June 5, 2024 09:02

Merge branch 'main' into cleanup-otel-provider

34102f2

# Conflicts: # src/main/java/io/jenkins/plugins/opentelemetry/JenkinsOpenTelemetryPluginConfiguration.java # src/main/java/io/jenkins/plugins/opentelemetry/OpenTelemetrySdkProvider.java

cyrille-leclerc dismissed kuisathaverat’s stale review via 34102f2 June 17, 2024 11:02

cyrille-leclerc marked this pull request as ready for review June 17, 2024 11:11

cyrille-leclerc changed the title ~~Refactor the OTelSdkProvider into an implementation of openTelemetry~~ Refactor the OTelSdkProvider into an implementation of OpenTelemetry Jun 17, 2024

cyrille-leclerc mentioned this pull request Jun 17, 2024

Link OpenTelemetry traces to parent span when publishing test results jenkinsci/junit-sql-storage-plugin#414

Draft

6 tasks

Merge branch 'main' into cleanup-otel-provider

a702d36

kuisathaverat previously approved these changes Jun 20, 2024

View reviewed changes

Add reconfigurability of the TraceProvider, LoggerProvider, and Event…

d0c9ca4

…LoggerProvider Signed-off-by: Cyrille Le Clerc <[email protected]>

cyrille-leclerc dismissed kuisathaverat’s stale review via d0c9ca4 June 20, 2024 21:30

cyrille-leclerc requested a review from kuisathaverat June 21, 2024 07:22

kuisathaverat reviewed Jun 21, 2024

View reviewed changes

kuisathaverat approved these changes Jun 21, 2024

View reviewed changes

cyrille-leclerc merged commit d997f5f into main Jun 21, 2024
14 checks passed

cyrille-leclerc deleted the cleanup-otel-provider branch June 21, 2024 09:32

cyrille-leclerc mentioned this pull request Jun 21, 2024

Don't add nudge to define visualisation url if plugin isn't configured #868

Closed

6 tasks

timja reviewed Jun 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the `OTelSdkProvider` into an implementation of `OpenTelemetry` #704

Refactor the `OTelSdkProvider` into an implementation of `OpenTelemetry` #704

cyrille-leclerc commented Sep 15, 2023 •

edited

Loading

Submitter checklist

cyrille-leclerc commented Jun 20, 2024

kuisathaverat Jun 21, 2024

kuisathaverat Jun 21, 2024

kuisathaverat Jun 21, 2024

kuisathaverat Jun 21, 2024

kuisathaverat Jun 21, 2024

kuisathaverat left a comment

cyrille-leclerc commented Jun 21, 2024

timja Jun 21, 2024

cyrille-leclerc Jun 21, 2024

timja Jun 21, 2024

Refactor the OTelSdkProvider into an implementation of OpenTelemetry #704

Refactor the OTelSdkProvider into an implementation of OpenTelemetry #704

Conversation

cyrille-leclerc commented Sep 15, 2023 • edited Loading

Testing done

Submitter checklist

cyrille-leclerc commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuisathaverat left a comment

Choose a reason for hiding this comment

cyrille-leclerc commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor the `OTelSdkProvider` into an implementation of `OpenTelemetry` #704

Refactor the `OTelSdkProvider` into an implementation of `OpenTelemetry` #704

cyrille-leclerc commented Sep 15, 2023 •

edited

Loading