Added step withSpanAttributes to set the attributes also on child spans, added setSpanAttributes for setting the attribute only on the target #827

christophe-kamphaus-jemmic · 2024-03-19T01:19:41Z

The withSpanAttributes step was added. This new step sets the attributes on all child spans created inside the body block. This is implemented using the StepContext.

setSpanAttributes was added to set attribute on the target without a body block similar to the existing withSpanAttribute.
setSpanAttributes is preferred over withSpanAttribute for consistency with other plugins (eg. withCredentials).

Fixes #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 setSpanAttributes / withSpanAttributes might be missed.
For example closed spans or some open child spans.
Child spans created after the execution of withSpanAttributes will all have the attribute set correctly.

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

Potential future work (not in this PR)

A step to create a new span (Capability to manually create spans #90)

Testing done

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

Submitter checklist

Give feedback

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

This parameter is not yet functional, but it should be accepted in a Jenkinsfile.

…nAttributeStep This is to make it clear that it's testing the target parameter. An additional withSpanAttribute asserts that the root span can be edited from anywhere in the pipeline.

…nAttributeStepSetOn The test pipeline sets the parameter setOn.

The asserts for setting of attributes on children (not yet implemented) are commented out.

Also we rename some variables to align with the attribute key being tested

This makes it possible to distinguish setAttribute performed by withSpanAttribute and later setAttributes for child spans.

christophe-kamphaus-jemmic · 2024-03-19T01:37:27Z

@cyrille-leclerc would you mind taking a look?

christophe-kamphaus-jemmic · 2024-03-19T01:42:34Z

~~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 · 2024-03-19T09:35:49Z

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

src/main/java/io/jenkins/plugins/opentelemetry/job/OtelTraceService.java

kuisathaverat · 2024-03-19T10:06:45Z

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 · 2024-03-21T23:07:32Z

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.

This will prevent more expensive iterations and listing of all actions of type AttributeSetterAction. Also replace AttributeSetterAction by the existing OpenTelemetryAttributesAction. OpenTelemetryAttributesAction was not yet used for Run, Build, FlowNode instances.

depending on whether it takes a closure (and sets the attribute on child spans as well) or not. Also splits the tests accordingly (WIP).

…ttribute

This way no span will be created for it same as for withSpanAttribute.

Instead of setting child span attributes from this action, the context's OpenTelemetryAttributesAction is used. We keep it only for setting it for the "Phase: Finalise" span.

This will allow running it in the declarative pipeline options

christophe-kamphaus-jemmic · 2024-04-15T15:46:10Z

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

…ttribute This ensures backwards compatibility of existing pipelines.

christophe-kamphaus-jemmic · 2024-04-18T19:36:15Z

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?

Only do this if asked for in PR review.

src/main/java/io/jenkins/plugins/opentelemetry/job/OtelTraceService.java

cyrille-leclerc · 2024-04-20T19:37:45Z

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

kuisathaverat · 2024-04-24T11:30:10Z

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

christophe-kamphaus-jemmic · 2024-05-08T12:43:39Z

Any update @kuisathaverat @cyrille-leclerc ?

kuisathaverat · 2024-05-08T14:56:05Z

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.

christophe-kamphaus-jemmic added 16 commits March 19, 2024 02:01

173 Add parameter setOn to the withSpanAttribute step (WIP)

0a05828

This parameter is not yet functional, but it should be accepted in a Jenkinsfile.

173 Rename + improve existing test case testSimplePipelineWithWithSpa…

69d596a

…nAttributeStep This is to make it clear that it's testing the target parameter. An additional withSpanAttribute asserts that the root span can be edited from anywhere in the pipeline.

173 Added test case for setOn parameter testSimplePipelineWithWithSpa…

aa9775a

…nAttributeStepSetOn The test pipeline sets the parameter setOn.

173 Added asserts for setOn test case

2da15a2

The asserts for setting of attributes on children (not yet implemented) are commented out.

173 Use assert reason instead of comments

ea01c45

173 Implement attribute setting on child nodes

34104af

173 Enable test for child attributes

9173cd8

Also we rename some variables to align with the attribute key being tested

174 Assert attribute on phases

e953800

173 Added assert for closed child span

97d407d

173 Add test for block use of withSpanAttribute

dbac5f9

173 Add test for checking overridden attribute keys

264344a

173 Add comment about overriding attributes

95c1ace

173 Fix error "Failed to serialize AttributeSetterAction#attributeKey"

7df888f

173 Improve comment

e4d3464

173 Improved logging

5db2e48

This makes it possible to distinguish setAttribute performed by withSpanAttribute and later setAttributes for child spans.

173 Code style

6a4eabf

christophe-kamphaus-jemmic requested a review from kuisathaverat as a code owner March 19, 2024 01:19

kuisathaverat reviewed Mar 19, 2024

View reviewed changes

src/main/java/io/jenkins/plugins/opentelemetry/job/OtelTraceService.java Outdated Show resolved Hide resolved

christophe-kamphaus-jemmic added 7 commits March 26, 2024 19:53

173 Code style

563d354

173 Split withAttribute into withSpanAttribute and setSpanAttribute

9acb9fc

depending on whether it takes a closure (and sets the attribute on child spans as well) or not. Also splits the tests accordingly (WIP).

173 Small code style fixes

68bb552

173 Adapt previous test to use withSpanAttribute closure and setSpanA…

d14e9a6

…ttribute

173 Add setSpanAttribute to the list of ignored steps

812841f

This way no span will be created for it same as for withSpanAttribute.

173 Remove unnecessary use of the Run's OpenTelemetryAttributesAction

250c1e0

Instead of setting child span attributes from this action, the context's OpenTelemetryAttributesAction is used. We keep it only for setting it for the "Phase: Finalise" span.

christophe-kamphaus-jemmic added 3 commits March 29, 2024 16:08

173 Add step withSpanAttributes

6b2a8fc

This will allow running it in the declarative pipeline options

173 Allow specifying multiple span attributes

740941a

173 Improve error message on null value

5c5094b

christophe-kamphaus-jemmic added 3 commits April 18, 2024 21:01

173 change todo comment, this has been implemented

4d30816

173 Change setSpanAttribute to setSpanAttributes and revert withSpanA…

c67b4b8

…ttribute This ensures backwards compatibility of existing pipelines.

173 Document the new spanAttribute steps

b8cff39

christophe-kamphaus-jemmic added 4 commits April 18, 2024 21:37

173 Remove unused import

ef8641a

173 Improve comment

4af7ffb

173 Fix SpotBugs issue

9d20436

173 Revert change to mark withSpanAttribute as deprecated

bddf07e

Only do this if asked for in PR review.

christophe-kamphaus-jemmic requested a review from kuisathaverat April 18, 2024 20:24

christophe-kamphaus-jemmic mentioned this pull request Apr 18, 2024

Support defining attributes at the job levels that are added to spans #173

Closed

kuisathaverat added the enhancement New feature or request label Apr 19, 2024

cyrille-leclerc reviewed Apr 20, 2024

View reviewed changes

src/main/java/io/jenkins/plugins/opentelemetry/job/OtelTraceService.java Show resolved Hide resolved

christophe-kamphaus-jemmic and others added 2 commits April 22, 2024 11:06

173 Add comments

c26ed95

Merge branch 'main' into 173-set-attributes-on-child-spans

d199e7a

christophe-kamphaus-jemmic mentioned this pull request May 16, 2024

Optionally include pipeline build span attributes in related step spans #811

Closed

kuisathaverat approved these changes May 29, 2024

View reviewed changes

kuisathaverat merged commit 862c5d2 into jenkinsci:main May 29, 2024
14 checks passed

christophe-kamphaus-jemmic deleted the 173-set-attributes-on-child-spans branch May 29, 2024 07:30

This was referenced Jul 2, 2024

Way to insert custom SpanProcessors and SpanExporters? #722

Closed

Add span traces steps to help with distributes traces in any Jenkins shared library #66

Closed

christophe-kamphaus-jemmic mentioned this pull request Oct 2, 2024

Build health metrics dimensions #952

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added step withSpanAttributes to set the attributes also on child spans, added setSpanAttributes for setting the attribute only on the target #827

Added step withSpanAttributes to set the attributes also on child spans, added setSpanAttributes for setting the attribute only on the target #827

christophe-kamphaus-jemmic commented Mar 19, 2024 •

edited

Loading

Submitter checklist

christophe-kamphaus-jemmic commented Mar 19, 2024

christophe-kamphaus-jemmic commented Mar 19, 2024 •

edited

Loading

kuisathaverat commented Mar 19, 2024

kuisathaverat commented Mar 19, 2024

christophe-kamphaus-jemmic commented Mar 21, 2024

christophe-kamphaus-jemmic commented Apr 15, 2024 •

edited

Loading

christophe-kamphaus-jemmic commented Apr 18, 2024

cyrille-leclerc commented Apr 20, 2024

kuisathaverat commented Apr 24, 2024

christophe-kamphaus-jemmic commented May 8, 2024

kuisathaverat commented May 8, 2024

Added step withSpanAttributes to set the attributes also on child spans, added setSpanAttributes for setting the attribute only on the target #827

Added step withSpanAttributes to set the attributes also on child spans, added setSpanAttributes for setting the attribute only on the target #827

Conversation

christophe-kamphaus-jemmic commented Mar 19, 2024 • edited Loading

Limitations

Potential future work (not in this PR)

Testing done

Submitter checklist

christophe-kamphaus-jemmic commented Mar 19, 2024

christophe-kamphaus-jemmic commented Mar 19, 2024 • edited Loading

kuisathaverat commented Mar 19, 2024

kuisathaverat commented Mar 19, 2024

christophe-kamphaus-jemmic commented Mar 21, 2024

christophe-kamphaus-jemmic commented Apr 15, 2024 • edited Loading

christophe-kamphaus-jemmic commented Apr 18, 2024

cyrille-leclerc commented Apr 20, 2024

kuisathaverat commented Apr 24, 2024

christophe-kamphaus-jemmic commented May 8, 2024

kuisathaverat commented May 8, 2024

christophe-kamphaus-jemmic commented Mar 19, 2024 •

edited

Loading

christophe-kamphaus-jemmic commented Mar 19, 2024 •

edited

Loading

christophe-kamphaus-jemmic commented Apr 15, 2024 •

edited

Loading