Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,12 @@
<artifactId>parsson</artifactId>
<version>1.1.7</version>
</dependency>
<dependency>
<!-- TODO remove incremental version when released -->
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-step-api</artifactId>
<version>707.v76364b_2b_6818</version>
</dependency>
</dependencies>
</dependencyManagement>
<dependencies>
Expand Down Expand Up @@ -377,6 +383,10 @@
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-multibranch</artifactId>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-step-api</artifactId>
</dependency>
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-definition</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,48 +6,30 @@
package io.jenkins.plugins.opentelemetry.init;

import hudson.Extension;
import hudson.util.ClassLoaderSanityThreadFactory;
import hudson.util.DaemonThreadFactory;
import hudson.util.NamingThreadFactory;
import io.jenkins.plugins.opentelemetry.api.OpenTelemetryLifecycleListener;
import io.opentelemetry.context.Context;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import java.lang.reflect.Field;
import java.util.Arrays;
import java.util.Optional;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nonnull;
import org.jenkinsci.plugins.workflow.steps.SynchronousNonBlockingStepExecution;

/**
* Initializes the instrumentation for {@link SynchronousNonBlockingStepExecution} by augmenting the
* {@link ExecutorService} to ensure that the OpenTelemetry context is propagated correctly.
*/
@Extension
public class StepExecutionInstrumentationInitializer implements OpenTelemetryLifecycleListener {
public class StepExecutionInstrumentationInitializer
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see 0c8bc8b

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@dwnusbaum dwnusbaum Aug 6, 2025

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you give context of this change?

Copy link
Member

Choose a reason for hiding this comment

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

@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.

implements SynchronousNonBlockingStepExecution.ExecutorServiceAugmentor {

static final Logger logger = Logger.getLogger(StepExecutionInstrumentationInitializer.class.getName());

@Override
public void afterConfiguration(@Nonnull ConfigProperties configProperties) {
try {
logger.log(
Level.FINE, () -> "Instrumenting " + SynchronousNonBlockingStepExecution.class.getName() + "...");
Class<SynchronousNonBlockingStepExecution> synchronousNonBlockingStepExecutionClass =
SynchronousNonBlockingStepExecution.class;
Arrays.stream(synchronousNonBlockingStepExecutionClass.getDeclaredFields())
.forEach(field -> logger.log(Level.FINE, () -> "Field: " + field.getName()));
Field executorServiceField = synchronousNonBlockingStepExecutionClass.getDeclaredField("executorService");
executorServiceField.setAccessible(true);
ExecutorService executorService = (ExecutorService) Optional.ofNullable(executorServiceField.get(null))
.orElseGet(() -> Executors.newCachedThreadPool(new NamingThreadFactory(
new ClassLoaderSanityThreadFactory(new DaemonThreadFactory()),
"org.jenkinsci.plugins.workflow.steps.SynchronousNonBlockingStepExecution")));
ExecutorService instrumentedExecutorService = Context.taskWrapping(executorService);
executorServiceField.set(null, instrumentedExecutorService);

// org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.runner
} catch (NoSuchFieldException | IllegalAccessException e) {
throw new RuntimeException(e);
}
/**
* Augment the provided ExecutorService to wrap it with OpenTelemetry context propagation
* @param executorService the ExecutorService to augment
* @return the augmented ExecutorService
* @see SynchronousNonBlockingStepExecution#getExecutorService()
*/
public ExecutorService augment(ExecutorService executorService) {
logger.log(Level.FINE, () -> "Instrumenting " + SynchronousNonBlockingStepExecution.class.getName() + "...");
return Context.taskWrapping(executorService);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package io.jenkins.plugins.opentelemetry;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.nullValue;

import hudson.model.Result;
import io.jenkins.plugins.opentelemetry.init.StepExecutionInstrumentationInitializer;
import io.opentelemetry.sdk.common.CompletableResultCode;
import io.opentelemetry.sdk.testing.exporter.InMemorySpanExporterProvider;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.TimeUnit;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.JenkinsRule;

public class JenkinsOtelPluginNoConfigurationTest {

@Rule
public JenkinsRule j = new JenkinsRule();

@Rule
public BuildWatcher buildWatcher = new BuildWatcher();

/**
* Test that the StepExecutionInstrumentationInitializer does nothing when configuration is not set.
* This test is similar to {@link JenkinsOtelPluginIntegrationTest#testSpanContextPropagationSynchronousNonBlockingTestStep()}
*/
@Test
public void testNoOpWhenNotConfigured() throws Exception {

String pipelineScript =
"""
node() {
stage('ze-stage1') {
echo message: 'hello'
spanContextPropagationSynchronousNonBlockingTestStep()
}
}""";
j.createOnlineSlave();
final String jobName = "test-SpanContextPropagationSynchronousTestStep";
WorkflowJob pipeline = j.createProject(WorkflowJob.class, jobName);
pipeline.setDefinition(new CpsFlowDefinition(pipelineScript, true));
j.assertBuildStatus(Result.SUCCESS, pipeline.scheduleBuild2(0));

CompletableResultCode result = JenkinsControllerOpenTelemetry.get()
.getOpenTelemetrySdk()
.getSdkTracerProvider()
.forceFlush();
result.join(1, TimeUnit.SECONDS);

// without specific configuration, no spans should be exported
assertThat(InMemorySpanExporterProvider.LAST_CREATED_INSTANCE, nullValue());
}

/**
* Make sure a standard pipeline with synchronous non-blocking steps works with {@link StepExecutionInstrumentationInitializer#augment(ExecutorService)}
*/
@Test
public void testStandardPipeline() throws Exception {
j.createOnlineSlave();
WorkflowJob pipeline = j.createProject(WorkflowJob.class);
pipeline.setDefinition(new CpsFlowDefinition(
"""
node {
writeFile(file: 'file', text: 'Hello, World!')
archiveArtifacts('file')
}
""",
true));

var build = j.assertBuildStatus(Result.SUCCESS, pipeline.scheduleBuild2(0));
assertThat(build.getArtifacts(), hasSize(1));
}
}

This file was deleted.

Loading