Remove reflection and augment the `SynchronousNonBlockingStepExecution#executorService` by jgreffe · Pull Request #1139 · jenkinsci/opentelemetry-plugin

jgreffe · 2025-06-30T17:01:45Z

Code refactoring to avoid using reflection against workflow-step-api-plugin

Uses the new API provided by jenkinsci/workflow-step-api-plugin#226

Testing done

Submitter checklist

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 demonstrate the feature works or the issue is fixed

…n#executorService`

src/test/java/io/jenkins/plugins/opentelemetry/JenkinsOtelPluginIntegrationTest.java

dwnusbaum · 2025-07-01T17:55:21Z

...main/java/io/jenkins/plugins/opentelemetry/init/StepExecutionInstrumentationInitializer.java

-public class StepExecutionInstrumentationInitializer implements OpenTelemetryLifecycleListener {
-
-    final static Logger logger = Logger.getLogger(StepExecutionInstrumentationInitializer.class.getName());
+public class StepExecutionInstrumentationInitializer


Note that this now happens unconditionally when the plugin is installed, not just when OTel is configured. I think that's ok and Context.taskWrapping will just be a no-op if the plugin isn't configured, but can you check to confirm?

The fact that SynchronousNonBlockingStepExecution.executorService is only initialized once though means that this won't work when the OTel plugin is installed dynamically. Maybe that's ok, but it would probably need to be documented. We could probably make things work for dynamic installations too if necessary, but it would be more complicated.

Please see 0c8bc8b

any reason to change the signature of the class https://github.com/jenkinsci/opentelemetry-api-plugin/blob/main/src/main/java/io/jenkins/plugins/opentelemetry/api/OpenTelemetryLifecycleListener.java

The signature has changed because the new extension point in workflow-step-api is unrelated to OpenTelemetryLifecycleListener. It applies unconditionally, which is why Julien added JenkinsOtelPluginNoConfigurationTest, to show that things work ok when this plugin is installed but not configured because Context.taskWrapping is simply a no-op in that case.

We could keep OpenTelemetryLifecycleListener in the signature here, but the afterConfiguration method would not do anything. The new upstream API does not allow dynamic reconfiguration of the ExecutorService (this is related to the issue with dynamic plugin installations I mention above).

If it's critical to support context propagation for synchronous steps after dynamic installations of the OTel plugin, we can probably make it work, but the API would need to be more complicated.

@kuisathaverat I understand you want to wait to review this PR until it is in review, but we are introducing the upstream API specifically to be able to remove the existing reflection in this class, and there are no other expected consumers. We don't want to release the new API until we know that you are generally ok with how it works. Can you please review this comment thread when you have some time and let us know if you are ok with the general idea of the change?

This comment mentions a limitation of the change, which I want to know if you are ok with or not. We can likely adapt the API to avoid this limitation, but it will make things a bit more complicated.

This comment explains the reason for the signature change.

src/test/java/io/jenkins/plugins/opentelemetry/JenkinsOtelPluginNoConfigurationTest.java

Signed-off-by: Julien Greffe <jgreffe@cloudbees.com>

kuisathaverat · 2025-07-11T14:12:14Z

pom.xml

+            <dependency>
+                <groupId>org.jenkins-ci.plugins.workflow</groupId>
+                <artifactId>workflow-step-api</artifactId>
+                <version>705.vd44f505b_0c5b_</version>


this is a test dependency only

@kuisathaverat Do you mean that you think this should be test scope, or that this is an incremental version of the plugin and we need to wait for a release?

I agree with the latter, @jgreffe in this kind of case where you introduce a new API and need to open a downstream PR to demonstrate the use case and make sure everything works, I recommend keeping the downstream PR in draft state and adding a comment above the incremental version like "TODO: Wait for release of <link to upstream PR>", so that no one accidentally merges the downstream PR too soon.

If you were talking about making this test scope though, opentelemetry currently has compile scope dependencies on workflow-job, workflow-cps, workflow-basic-steps, pipeline-model-definition, and various other plugins for Pipeline steps, all of which have either direct or transitive compile scope dependencies on workflow-step-api. workflow-step-api is at the bottom of the dependency chain of all the Pipeline-related plugins.

workflow-step-api is also is used directly in this plugin in a few ways, such as the step implementations in WithNewSpanStep, WithSpanAttributeStep, and WithSpanAttributesStep, and StepExecutionInstrumentationInitializer. Even if it's just for clarity, this plugin should probably have a direct dependency on workflow-step-api and workflow-api.

pom.xml

kuisathaverat · 2025-07-11T14:15:08Z

...main/java/io/jenkins/plugins/opentelemetry/init/StepExecutionInstrumentationInitializer.java

-public class StepExecutionInstrumentationInitializer implements OpenTelemetryLifecycleListener {
-
-    final static Logger logger = Logger.getLogger(StepExecutionInstrumentationInitializer.class.getName());
+public class StepExecutionInstrumentationInitializer


Can you give context of this change?

@kuisathaverat Sorry, I thought someone had already contacted you to provide more context, but in general, CloudBees is looking into adding opentelemetry and opentelemetry-api to our CloudBees Assurance Program. As part of this process, we reviewed the codebase to find things that seemed problematic from a stability and maintenance standpoint. This is why CloudBees developers have been looking at removing reflection in this PR and in #1137 as well as investigating other issues such as #700, #1045, #1088, and #1151.

For this change in particular, I think the description here describes the goal - we just want to get rid of reflection. The description of jenkinsci/workflow-step-api-plugin#226 gives further context and lists some other approaches that we thought about briefly. If you think some other approach would be better, we'd be happy to look into it.

src/test/java/io/jenkins/plugins/opentelemetry/JenkinsOtelPluginIntegrationTest.java

kuisathaverat · 2025-07-11T14:21:24Z

I do not see a clear goal in these changes, but they are not related to the context given in the PR description, and there are a few changes that are not related to each other. Please, provide a proper description of the change. If there are different purposes, create different PRs; any style change should be put in a different PR. In short, I have a limited time for OSS, I want to use it in things that matter.

kuisathaverat

Limit the changes to those described in the PR description

src/test/java/io/jenkins/plugins/opentelemetry/BaseIntegrationTest.java

kuisathaverat · 2025-07-11T16:45:09Z

src/test/java/io/jenkins/plugins/opentelemetry/JenkinsOtelPluginNoConfigurationTest.java

+     * This test is similar to {@link JenkinsOtelPluginIntegrationTest#testSpanContextPropagationSynchronousNonBlockingTestStep()}
+     */
+    @Test
+    public void test_noOp_when_not_configured() throws Exception {


camelcase is not the name convention used in the plugin

src/test/java/io/jenkins/plugins/opentelemetry/JenkinsOtelPluginNoConfigurationTest.java

.../java/io/jenkins/plugins/opentelemetry/init/StepExecutionInstrumentationInitializerTest.java

kuisathaverat · 2025-07-11T17:18:43Z

src/test/java/io/jenkins/plugins/opentelemetry/JenkinsOtelPluginNoConfigurationTest.java

+    /**
+     * As the JVM and classes are loaded only once for the whole test, {@link SynchronousNonBlockingStepExecution#getExecutorService()} augments only once. The current boolean keeps track of the augmentation status.
+     */
+    private static boolean augmented = false;


I am not sure what the goal of this boolean; it depends on the order of execution of the test, and it will be false/true when the test arrives at the call of checkAugmentation, I mean you have test A and B both call checkAugmentation, and augmented is false when the class is intanciated. If the test A run calls checkAugmentation with augmented=false and changes the value to true, the assert passes. Then, the test B will call checkAugmentation with augmented=true. If you invert the order of the test (B then A), B will call checkAugmentation with augmented=false and A will call checkAugmentation with augmented=true that's completely the opposite.

Yes, it's the purpose of this boolean, we just want to check augmentation happened once.

I think if we want to test that the augmentation only happens once, we should do it in workflow-step-api, since that plugin is responsible for the logic that results in it only happening once.

For this class, only keeping testStandardPipeline should be good enough to demonstrate synchronous steps are not broken when OTel is installed but not configured. The other tests seem ok to delete.

Please see 99b331d

And upstream jenkinsci/workflow-step-api-plugin@2947eae

...main/java/io/jenkins/plugins/opentelemetry/init/StepExecutionInstrumentationInitializer.java

Signed-off-by: Julien Greffe <jgreffe@cloudbees.com> # Conflicts: # pom.xml # src/main/java/io/jenkins/plugins/opentelemetry/init/StepExecutionInstrumentationInitializer.java # src/test/java/io/jenkins/plugins/opentelemetry/BaseIntegrationTest.java # src/test/java/io/jenkins/plugins/opentelemetry/JenkinsOtelPluginIntegrationTest.java # src/test/java/io/jenkins/plugins/opentelemetry/init/StepExecutionInstrumentationInitializerTest.java

Signed-off-by: Julien Greffe <jgreffe@cloudbees.com>

src/test/java/io/jenkins/plugins/opentelemetry/JenkinsOtelPluginIntegrationTest.java

.../java/io/jenkins/plugins/opentelemetry/init/StepExecutionInstrumentationInitializerTest.java

dwnusbaum · 2025-07-24T15:58:16Z

src/test/java/io/jenkins/plugins/opentelemetry/JenkinsOtelPluginNoConfigurationTest.java

+    /**
+     * As the JVM and classes are loaded only once for the whole test, {@link SynchronousNonBlockingStepExecution#getExecutorService()} augments only once. The current boolean keeps track of the augmentation status.
+     */
+    private static boolean augmented = false;


I think if we want to test that the augmentation only happens once, we should do it in workflow-step-api, since that plugin is responsible for the logic that results in it only happening once.

For this class, only keeping testStandardPipeline should be good enough to demonstrate synchronous steps are not broken when OTel is installed but not configured. The other tests seem ok to delete.

dwnusbaum · 2025-07-24T17:32:02Z

...main/java/io/jenkins/plugins/opentelemetry/init/StepExecutionInstrumentationInitializer.java

-public class StepExecutionInstrumentationInitializer implements OpenTelemetryLifecycleListener {
-
-    final static Logger logger = Logger.getLogger(StepExecutionInstrumentationInitializer.class.getName());
+public class StepExecutionInstrumentationInitializer


The signature has changed because the new extension point in workflow-step-api is unrelated to OpenTelemetryLifecycleListener. It applies unconditionally, which is why Julien added JenkinsOtelPluginNoConfigurationTest, to show that things work ok when this plugin is installed but not configured because Context.taskWrapping is simply a no-op in that case.

We could keep OpenTelemetryLifecycleListener in the signature here, but the afterConfiguration method would not do anything. The new upstream API does not allow dynamic reconfiguration of the ExecutorService (this is related to the issue with dynamic plugin installations I mention above).

If it's critical to support context propagation for synchronous steps after dynamic installations of the OTel plugin, we can probably make it work, but the API would need to be more complicated.

...main/java/io/jenkins/plugins/opentelemetry/init/StepExecutionInstrumentationInitializer.java

kuisathaverat · 2025-07-27T10:30:23Z

I will wait for the PR to be in review to take a new look at it.

Signed-off-by: Julien Greffe <jgreffe@cloudbees.com>

…into jgreffe/avoid-steps-reflection

Remove reflection and augment the `SynchronousNonBlockingStepExecutio…

bae8938

…n#executorService`

jgreffe requested a review from kuisathaverat as a code owner June 30, 2025 17:01

jgreffe changed the title ~~Remove reflection and augment the `SynchronousNonBlockingStepExecutio…~~ Remove reflection and augment the SynchronousNonBlockingStepExecution#executorService Jun 30, 2025

jgreffe mentioned this pull request Jun 30, 2025

Add SynchronousNonBlockingStepExecution.ExecutorServiceAugmentor extension point jenkinsci/workflow-step-api-plugin#226

Draft

6 tasks

dwnusbaum reviewed Jul 1, 2025

View reviewed changes

jgreffe added 2 commits July 2, 2025 04:08

Minimal changes

0149926

Additional tests

0c8bc8b

jgreffe commented Jul 2, 2025

View reviewed changes

src/test/java/io/jenkins/plugins/opentelemetry/JenkinsOtelPluginNoConfigurationTest.java Show resolved Hide resolved

jgreffe requested a review from dwnusbaum July 2, 2025 08:16

jgreffe added 2 commits July 9, 2025 16:04

Additional unit test

923cc99

Signed-off-by: Julien Greffe <jgreffe@cloudbees.com>

Additional unit test

3343a75

Signed-off-by: Julien Greffe <jgreffe@cloudbees.com>