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,43 +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 | ||
|
Member
There was a problem hiding this comment. Note that this now happens unconditionally when the plugin is installed, not just when OTel is configured. I think that's ok and The fact that
Contributor
There was a problem hiding this comment. 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
Member
There was a problem hiding this comment. The signature has changed because the new extension point in We could keep 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.
Member
There was a problem hiding this comment. @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.
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 test_noOp_when_not_configured() 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 test_standard_pipeline() throws Exception { | ||
jgreffe marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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.
this is a test dependency only
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 Do you mean that you think this should be
testscope, 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
testscope though,opentelemetrycurrently hascompilescope dependencies onworkflow-job,workflow-cps,workflow-basic-steps,pipeline-model-definition, and various other plugins for Pipeline steps, all of which have either direct or transitivecompilescope dependencies onworkflow-step-api.workflow-step-apiis at the bottom of the dependency chain of all the Pipeline-related plugins.workflow-step-apiis also is used directly in this plugin in a few ways, such as the step implementations inWithNewSpanStep,WithSpanAttributeStep, andWithSpanAttributesStep, andStepExecutionInstrumentationInitializer. Even if it's just for clarity, this plugin should probably have a direct dependency onworkflow-step-apiandworkflow-api.