Remove reflection and augment the SynchronousNonBlockingStepExecution#executorService
#1139
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,48 +6,20 @@ | |
| package io.jenkins.plugins.opentelemetry.init; | ||
|
|
||
| import hudson.Extension; | ||
| import io.opentelemetry.context.Context; | ||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.logging.Level; | ||
| import java.util.logging.Logger; | ||
| import org.jenkinsci.plugins.workflow.steps.SynchronousNonBlockingStepExecution; | ||
|
|
||
| @Extension | ||
| public class StepExecutionInstrumentationInitializer | ||
|
Contributor
There was a problem hiding this comment. Can you give context of this change?
Member
There was a problem hiding this comment. @kuisathaverat Sorry, I thought someone had already contacted you to provide more context, but in general, CloudBees is looking into adding 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()); | ||
|
|
||
| public ExecutorService augment(ExecutorService executorService) { | ||
jgreffe marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| logger.log(Level.FINE, () -> "Instrumenting " + SynchronousNonBlockingStepExecution.class.getName() + "..."); | ||
| return Context.taskWrapping(executorService); | ||
| } | ||
| } | ||
jgreffe marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| 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 java.util.logging.Level; | ||
| import org.hamcrest.Matchers; | ||
| import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; | ||
| import org.jenkinsci.plugins.workflow.job.WorkflowJob; | ||
| import org.jenkinsci.plugins.workflow.steps.SynchronousNonBlockingStepExecution; | ||
| import org.junit.Rule; | ||
| import org.junit.Test; | ||
| import org.jvnet.hudson.test.BuildWatcher; | ||
| import org.jvnet.hudson.test.JenkinsRule; | ||
| import org.jvnet.hudson.test.LogRecorder; | ||
|
|
||
| public class JenkinsOtelPluginNoConfigurationTest { | ||
|
|
||
| @Rule | ||
| public JenkinsRule j = new JenkinsRule(); | ||
|
|
||
| @Rule | ||
| public BuildWatcher buildWatcher = new BuildWatcher(); | ||
|
|
||
| /** | ||
| * 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; | ||
|
||
|
|
||
| /** | ||
| * 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)); | ||
|
|
||
| try (LogRecorder recorder = new LogRecorder() | ||
| .quiet() | ||
| .record(StepExecutionInstrumentationInitializer.class, Level.FINE) | ||
| .capture(10)) { | ||
| j.assertBuildStatus(Result.SUCCESS, pipeline.scheduleBuild2(0)); | ||
| checkAugmentation(recorder); | ||
| } | ||
| 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()); | ||
jgreffe marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /** | ||
| * 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)); | ||
|
|
||
| try (LogRecorder recorder = new LogRecorder() | ||
| .quiet() | ||
| .record(StepExecutionInstrumentationInitializer.class, Level.FINE) | ||
| .capture(10)) { | ||
| var build = j.assertBuildStatus(Result.SUCCESS, pipeline.scheduleBuild2(0)); | ||
| assertThat(build.getArtifacts(), hasSize(1)); | ||
| checkAugmentation(recorder); | ||
| } | ||
| } | ||
|
|
||
| private void checkAugmentation(LogRecorder recorder) { | ||
| if (!augmented) { | ||
| assertThat( | ||
| recorder.getMessages(), | ||
| Matchers.hasItem("Instrumenting " + SynchronousNonBlockingStepExecution.class.getName() + "...")); | ||
| augmented = true; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
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.taskWrappingwill just be a no-op if the plugin isn't configured, but can you check to confirm?The fact that
SynchronousNonBlockingStepExecution.executorServiceis 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see 0c8bc8b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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-apiis unrelated toOpenTelemetryLifecycleListener. It applies unconditionally, which is why Julien addedJenkinsOtelPluginNoConfigurationTest, to show that things work ok when this plugin is installed but not configured becauseContext.taskWrappingis simply a no-op in that case.We could keep
OpenTelemetryLifecycleListenerin the signature here, but theafterConfigurationmethod would not do anything. The new upstream API does not allow dynamic reconfiguration of theExecutorService(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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.