Fix: traceparent variable issue - #1219
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the TRACEPARENT environment variable was not updating when entering new stages or nodes in Jenkins pipelines. The fix changes the implementation to use the current active span from the thread instead of always using the root span of the build.
Changes:
- Modified
OtelEnvironmentContributorto useSpan.current()to get the active span on the thread, with a fallback to the run's root span if no valid span is found - Migrated
ConfigurationKeyTestfrom JUnit 4 to JUnit 5 assertions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/main/java/io/jenkins/plugins/opentelemetry/job/OtelEnvironmentContributor.java | Updated to use Span.current() instead of otelTraceService.getSpan(run) to capture the active span context, enabling TRACEPARENT to change between stages/nodes |
| src/test/java/io/jenkins/plugins/opentelemetry/semconv/ConfigurationKeyTest.java | Migrated from JUnit 4 (org.junit.Assert) to JUnit 5 (org.junit.jupiter.api.Assertions) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6823eba to
03618fd
Compare
03618fd to
b72cbd8
Compare
|
sorry for this, don't know how but i ran |
|
The change needs a unit test to verify that the traces are correct and that the order is as expected. |
|
@kuisathaverat I attempted to add a regression test using JenkinsRule and tried both Controller execution and a DummySlave agent, but the test fails to propagate the OpenTelemetry Context correctly in the test harness. |
The JenkinsRule works as a regular Jenkins (mostly). We have tests that verify traces in memory in other places, and the tests should be more or less similar. Without a unit test, we cannot validate the change. |
okay, working on it again !! |
so i have added a unit test file in it! |
| mockedSpan.when(Span::current).thenReturn(currentSpan); | ||
| contributor.buildEnvironmentFor(run, envVars, listener); | ||
| // Verification | ||
| verify(environmentContributorService).addEnvironmentVariables(run, envVars, currentSpan); |
There was a problem hiding this comment.
The test does not verify the current change in the behavior.
| mockedSpan.when(Span::current).thenReturn(invalidSpan); | ||
| contributor.buildEnvironmentFor(run, envVars, listener); | ||
| // Verification | ||
| verify(environmentContributorService).addEnvironmentVariables(run, envVars, rootSpan); |
There was a problem hiding this comment.
The test does not verify the current change in the behavior.
|
I've updated the test to be more explicit about the behavior change. |
| verify(environmentContributorService).addEnvironmentVariables(run, envVars, currentSpan); | ||
|
|
||
| // PROOF OF CHANGE | ||
| verify(otelTraceService, never()).getSpan(any()); |
There was a problem hiding this comment.
It verifies that the method otelTraceService.getSpan(...) is never called when the current span is valid. But do not verify the spans of the step is the parant span and not the root span
| verify(environmentContributorService).addEnvironmentVariables(run, envVars, rootSpan); | ||
|
|
||
| // Verify that it was actually called the fallback service | ||
| verify(otelTraceService).getSpan(run); |
|
I've updated the test to strictly verify the span identity. |
|
@kuisathaverat, Gentle ping |
|
The requested changes on testing are not addressed. The test provided do not verify the context pass between layers in a pipeline. |
|
Thanks for the feedback @kuisathaverat |
|
Manual test does not remove the need of a proper test. |
ef25e0f to
7c1d73d
Compare
|
@kuisathaverat Thanks for the guidance. I have updated the PR with a new dedicated integration test that addresses your concerns.
|
2130fc2 to
02f6e9d
Compare
52a9bab to
cc87aea
Compare
|
@kuisathaverat Thanks for your review, this new test file created by me adds a regression test that validates TRACEPARENT propagation and span creation across nested pipeline stages, if there are any changes that to be done pls specifically guide me, also it passes |
|
@kuisathaverat i have added an extra step |
…PipelineTest.java
|
Applied the suggestion and pushed the update! |
|
I need to verify an issue is not new (not related to this PR), before merging it and releasing it |
|
I am testing the incremental version on main and the incremental of this PR and the change cause #1234 |
|
I think the propagation now links Jenkins internal operations to the pipeline span. |
|
Hi @kuisathaverat and @Zenith1415, I reviewed this PR and the The concern about One possible approach to investigate: could we check This might prevent Jenkins internal spans from leaking into |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/test/java/io/jenkins/plugins/opentelemetry/job/TraceParentPipelineTest.java:93
extract()matches any line containing the prefix, which can accidentally capture the Jenkins+ echo ...command line rather than the actual output line. Withsh 'echo a_tp=$TRACEPARENT', the first match is typically+ echo a_tp=$TRACEPARENT, so the extracted value becomes$TRACEPARENTand the assertions become unreliable.
private String extract(String prefix, String log) {
for (String line : log.split("\n")) {
if (line.contains(prefix)) {
return line.substring(line.indexOf(prefix) + prefix.length()).trim();
}
}
return null;
src/test/java/io/jenkins/plugins/opentelemetry/job/TraceParentPipelineTest.java:78
- After switching the test to assert
env.TRACEPARENT, the current chain ofassertNotEquals(...)checks is too strict for the stated requirement (update between stages/nodes) and is likely to be flaky if the plugin uses a single stage span for multiple steps. The test should only require that TRACEPARENT changes between Stage-A and Stage-B, while still asserting all values share the same traceId.
assertNotEquals(aTp, bTp);
assertNotEquals(bTp, tryTp);
assertNotEquals(bTp, catchTp);
assertNotEquals(bTp, finalTp);
assertNotEquals(tryTp, catchTp);
assertNotEquals(tryTp, finalTp);
src/test/java/io/jenkins/plugins/opentelemetry/job/TraceParentPipelineTest.java:21
- This test sets
endpointtohttp://localhost:4317but never enablesOpenTelemetryConfiguration.TESTING_INMEMORY_MODE, so the plugin will configure the OTLP exporter and attempt to connect to a local collector during the test run. That makes the test environment-dependent and can introduce flaky failures or noisy logs; existing integration tests in this repo explicitly enable in-memory mode to avoid this.
public class TraceParentPipelineTest {
@Rule
public JenkinsRule jenkinsRule = new JenkinsRule();
src/test/java/io/jenkins/plugins/opentelemetry/job/TraceParentPipelineTest.java:52
- This test currently prints/extracts
$TRACEPARENTfromshsteps. The bug fixed in this PR is specifically about the Groovyenvobject (env.TRACEPARENT) not updating between stages; shell$TRACEPARENTis populated viaOtelStepEnvironmentContributorand can behave differently. To validate the fix, the pipeline should printenv.TRACEPARENTat each point being asserted.
This issue also appears in the following locations of the same file:
- line 72
- line 87
String pipelineScript = "node {\n" + " stage('Stage-A') {\n"
+ " sh 'echo a_tp=$TRACEPARENT'\n"
+ " }\n"
+ "\n"
+ " withEnv(['FOO=bar']) {\n"
+ " stage('Stage-B') {\n"
+ " sh 'echo b_tp=$TRACEPARENT'\n"
+ " try {\n"
+ " sh 'echo try_tp=$TRACEPARENT'\n"
+ " sh 'exit 1'\n"
+ " } catch (err) {\n"
+ " sh 'echo catch_tp=$TRACEPARENT'\n"
+ " } finally {\n"
+ " sh 'echo final_tp=$TRACEPARENT'\n"
+ " }\n"
+ " }\n"
+ " }\n"
+ "}";

Context
Fix for #1202.
Currently, the
TRACEPARENTenvironment variable injected into the Groovyenvobject is stuck on the Root Span of the build. It does not update when entering new stages or nodes, breaking distributed tracing context for scripts that rely onenv.TRACEPARENT.Changes
OtelEnvironmentContributor.javato useSpan.current()instead ofotelTraceService.getSpan(run).Testing done
Reproduced the issue locally with a pipeline script. verified that
env.TRACEPARENTnow changes between stages.Before Fix:

Stage 1:
00-abc...Stage 2:
00-abc...After Fix:

Stage 1:
00-abc...Stage 2:
00-xyz...