Change withSpanAttribute to set the attribute also on child spans, added setSpanAttribute for setting the attribute only on the target

[christophe-kamphaus-jemmic]

The withSpanAttribute step was changed to have takesImplicitBlockArgument() return true and to set the attribute on all spans created inside the body block. This is implemented using the StepContext.

setSpanAttribute was added to set attribute on the target without a body block.

Fixes https://github.com/jenkinsci/opentelemetry-plugin/issues/173

The main difficulty in implementing a solution for #173 was how to efficiently

• get child nodes of the target
  • I did not find an easy API for this.
  • I would have to either walk the whole flow graph or keep the span hierarchy somehow in memory. The OpenTelemetry API does not expose the parent context ID that I could find, so that was also not feasible.
  • In the end I just used the few otelTraceService.getSpan to set the attributes of some child spans of the root span and documented the limitations of the current implementation.
• remember the hierarchy of withSpanAttribute calls, so that new spans can efficiently set the required attributes
  • I solved this by adding the invisible action OpenTelemetryAttributesAction to the Run tracking child attributes for later agent and phase spans. The OpenTelemetryAttributesAction is added to the SpanContext to track attributes to be set to new spans within that context.
  • I adapted OpenTelemetryAttributesAction to be serializable to fix the error Failed to serialize OpenTelemetryAttributesAction.

Limitations

Some child spans that were created before the execution of setSpanAttribute / withSpanAttribute might be missed. For example closed spans or some open child spans. Child spans created after the execution of withSpanAttribute will all have the attribute set correctly.

Declarative pipelines can only use a single withSpanAttribute step inside the options block.

Potential future work (not in this PR)

• A step to create a new span (https://github.com/jenkinsci/opentelemetry-plugin/issues/90)

Testing done

• [x] I added JUnit tests to verify the desired behavior.
  • Added test verifying the behavior of withSpanAttribute
  • Added tests checks overriding behavior, ie. multiple withSpanAttribute steps with the same key.
• [x] Tested locally using the docker-compose in https://github.com/jenkinsci/opentelemetry-plugin/tree/main/demos
  • using withSpanAttribute(key: 'build.tool', value: 'gradle', target: 'PIPELINE_ROOT_SPAN') { ... } and verified in Jaeger that all expected spans have this tag.

### Submitter checklist
- [x] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!
- [x] Ensure that the pull request title represents the desired changelog entry
- [x] Please describe what you did
- [x] Link to relevant issues in GitHub or Jira
- [x] Link to relevant pull requests, esp. upstream and downstream changes
- [x] Ensure you have provided tests - that demonstrates feature works or fixes the issue

[christophe-kamphaus-jemmic]

@cyrille-leclerc would you mind taking a look?

[christophe-kamphaus-jemmic]

~~Should the new parameter setOn of the withSpanAttribute step be documented in a Markdown somewhere?~~

Should the new behavior of the withSpanAttribute step and the setSpanAttribute step be documented in a Markdown document somewhere?

[kuisathaverat]

get child nodes of the target I did not find an easy API for this. I would have to either walk the whole flow graph or keep the span hierarchy somehow in memory. The OpenTelemetry API does not expose the parent context ID that I could find, so that was also not feasible. In the end I just used the few otelTraceService.getSpan to set the attributes of some child spans of the root span and documented the limitations of the current implementation.

the trace.id identifies all the members in a transaction, so they are already related.

remember the hierarchy of withSpanAttribute calls, so that new spans can efficiently set the required attributes I solved this by adding invisible actions AttributeSetterAction to the Run and FlowNode's in a way that we can easily retrieve using the getAncestors method Is this a good way to do this? I noticed that these AttributeSetterAction are serialized. I refactor the code a bit to fix the error Failed to serialize AttributeSetterAction#attributeKey". Should AttributeSetterAction be serialized?

Serialized and memory consumption are the possible issues

[kuisathaverat]

Is this a good way to do this?

probably not; there are other steps like withCredentials that propagate something to lower levels in the pipeline like environment variables; they enclose the children into a closure to make it obvious that you are propagation something to your children, and I doubt they use actions to propagate the context.

https://www.jenkins.io/doc/pipeline/steps/credentials-binding/ https://github.com/jenkinsci/credentials-binding-plugin/blob/master/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java

[christophe-kamphaus-jemmic]

there are other steps like withCredentials that propagate something to lower levels in the pipeline like environment variables

I will take a look how they do it. Thanks for the links.

[christophe-kamphaus-jemmic]

Is this a good way to do this?

probably not; there are other steps like withCredentials that propagate something to lower levels in the pipeline like environment variables; they enclose the children into a closure to make it obvious that you are propagation something to your children, and I doubt they use actions to propagate the context.

https://www.jenkins.io/doc/pipeline/steps/credentials-binding/ https://github.com/jenkinsci/credentials-binding-plugin/blob/master/src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java

I changed the implementation to not use actions set on the FlowNodes for context propagation. Instead the StepContext now is used. This is also how BindingStep does it.

[christophe-kamphaus-jemmic]

Declarative pipelines can only use a single withSpanAttribute step inside the options block.

To fix this limitation and to improve backwards compatibility, I could leave the behavior of withSpanAttribute unchanged and add a withSpanAttributes step that takes a list of attributes to be set.

What do you think?

[christophe-kamphaus-jemmic]

Should the new behavior of the withSpanAttribute step and the setSpanAttribute step be documented in a Markdown document somewhere?

• To be updated at least in https://github.com/jenkinsci/opentelemetry-plugin/blob/main/docs/job-traces.md
• Search for withSpanAttribute in *.md files

DONE

[christophe-kamphaus-jemmic]

Declarative pipelines can only use a single withSpanAttribute step inside the options block.

To fix this limitation and to improve backwards compatibility, I could leave the behavior of withSpanAttribute unchanged and add a withSpanAttributes step that takes a list of attributes to be set.

What do you think?

After some reflection I've implemented this change.

withSpanAttribute is now backwards compatible. The new withSpanAttributes can be used to set several span attributes on child spans. This also fixes the limitation of declarative pipelines only allowing a single step of a given type in the options block.

For consistency with other plugins (eg. withCredentials) and to better distinguish the behavior to withSpanAttributes I've added setSpanAttributes for only setting the given attributes on the target.

Should withSpanAttribute be marked as deprecated with a recommendation to use setSpanAttributes instead?

[cyrille-leclerc]

Thank you for the effort, it took a long time. I suggested to add an explanation through javadocs.

[kuisathaverat]

The code LGTM, I have to find some time to make a manual test to verify there is nothing weird.

[christophe-kamphaus-jemmic]

Any update @kuisathaverat @cyrille-leclerc ?

[kuisathaverat]

I'm sorry. It is still on my plate, but I do not find time to test it. I will try to find some gap this week.