Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[maven-extension] Propagate OTel context to plugin mojos #786

Merged

Conversation

cyrille-leclerc
Copy link
Member

@cyrille-leclerc cyrille-leclerc commented Mar 14, 2023

Description:

Propagate OTel context to plugin mojos so they can add their own attributes and spans to traces.

Existing Issue(s):

None

Testing:

No unit test framework compatible with JUnit 5 for Maven builds yet.

Documentation:

See updated README.md

Outstanding items:

None

@@ -45,6 +47,8 @@ public final class OtelExecutionListener extends AbstractExecutionListener {

private static final Logger logger = LoggerFactory.getLogger(OtelExecutionListener.class);

private static final ThreadLocal<Scope> MOJO_EXECUTION_SCOPE = new ThreadLocal<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this cause any issue with parallel Maven builds with -T?

Copy link
Member Author

@cyrille-leclerc cyrille-leclerc Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, using a threadlocal from ExecutionListener#mojoStarted() to ExecutionListener#mojoSucceeded() / ExecutionListener#mojoFailed() works even when parallelising the Maven builds with -T.
The code contains state checks to ensure this assumption continues to be valid with newer versions of Maven parallelization.

I tested with 8 cores and -T 1C on a Macbok Air m2 2022 building https://github.com/apache/maven. I could confirm looking at the debug logs that the build was massively parallelised and that the threadlocal worked as desired.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added a comment in the code to explain the rationale.

@cyrille-leclerc cyrille-leclerc marked this pull request as ready for review March 14, 2023 21:44
@cyrille-leclerc cyrille-leclerc requested a review from a team March 14, 2023 21:44
@zoranzaric-cpx
Copy link

Works for me. Thanks a lot @cyrille-leclerc !

maven-extension/README.md Outdated Show resolved Hide resolved
maven-extension/README.md Outdated Show resolved Hide resolved
@mateuszrzeszutek mateuszrzeszutek merged commit 00a3928 into open-telemetry:main Mar 29, 2023
@cyrille-leclerc cyrille-leclerc deleted the otel-in-maven-plugins branch March 29, 2023 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants