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] Support Maven Parallel Builds #161

Merged
merged 1 commit into from
Jan 3, 2022

Conversation

cyrille-leclerc
Copy link
Member

@cyrille-leclerc cyrille-leclerc commented Jan 3, 2022

Description:

Support Maven Parallel Builds.

Existing Issue(s):

Testing:

Test the following build with the modified opentelemetry-maven-extension

otel-cli server tui
git clone https://github.com/apache/maven
cd maven
mvn dependency:copy -Dartifact=io.opentelemetry.contrib:opentelemetry-maven-extension:1.9.0-alpha
cp target/dependency/opentelemetry-maven-extension-1.9.0-alpha.jar /tmp/
mvn clean -Dotel.traces.exporter=otlp -Dotel.exporter.otlp.endpoint=http://localhost:4317 -Dmaven.ext.class.path=/tmp/opentelemetry-maven-extension-1.9.0-alpha.jar -T3

Documentation:

No change needed

Outstanding items:

None

@cyrille-leclerc cyrille-leclerc requested a review from a team January 3, 2022 14:24
@cyrille-leclerc cyrille-leclerc changed the title Support Maven Parallel Builds [Maven Extension] Support Maven Parallel Builds Jan 3, 2022
Copy link
Member

@kenfinnigan kenfinnigan left a comment

Choose a reason for hiding this comment

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

Looks reasonable, but had a question about a couple of new abstract methods

@@ -134,7 +148,9 @@ public static MavenProjectKey fromMavenProject(@Nonnull MavenProject mavenProjec

abstract String pluginArtifactId();

static MojoExecutionKey fromMojoExecution(MojoExecution mojoExecution) {
abstract MavenProjectKey projectKey();
Copy link
Member

Choose a reason for hiding this comment

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

Where is projectKey() used?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is used by the Map of spans by mojoExecutionKey of the SpanRegistry. It's needed when using Maven Parallel Builds as the same mojo goal can be executed simultaneously on multiple Maven sub projects.

See https://github.com/open-telemetry/opentelemetry-java-contrib/blob/v1.9.0/maven-extension/src/main/java/io/opentelemetry/maven/SpanRegistry.java#L23

@@ -114,9 +126,11 @@ public Span removeSpan(MojoExecution mojoExecution) {

abstract String artifactId();

public static MavenProjectKey fromMavenProject(@Nonnull MavenProject mavenProject) {
abstract String version();
Copy link
Member

Choose a reason for hiding this comment

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

Where is version() used?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a protection for the map of spans by MojoExecutionKeys. I imagine 2 mojo executions can't execute simultaneously on the same project with the different version of the Maven plugin but I felt it's a safe guardrail.

@kenfinnigan
Copy link
Member

@trask Is there a problem with the CODEOWNERS config as I expected to be one of the allowed approvers, which doesn't seem to be the case.

@trask
Copy link
Member

trask commented Jan 3, 2022

@trask Is there a problem with the CODEOWNERS config as I expected to be one of the allowed approvers, which doesn't seem to be the case.

ya, the problem is that:

The people you choose as code owners must have write permissions for the repository

(https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners)

I'm not sure what the best workaround is. For now I have given everyone in CODEOWNERS write permission to the repo, and I'll seek guidance in the otel community repo.

@trask trask merged commit 8cf7160 into open-telemetry:main Jan 3, 2022
@@ -38,6 +42,7 @@ public void setRootSpan(Span rootSpan) {
}

public Span getSpan(MavenProject mavenProject) {
logger.debug("OpenTelemetry: getSpan({}, {})", mavenProject, Thread.currentThread());
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should have Thread.currentThread in logging messages - most loggers will output the thread info if configured in the pattern anyways.

If we do have it we to check if debug level is enabled before calling the not trivially cheap method

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @anuraaga , fixed in PR #163

most loggers will output the thread info if configured in the pattern anyways.

Unfortunately it's not the case with the mvn --debug / mvn -X option but we can remove the thread name as we understood the behaviour of Maven Parallel Builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants