[maven-extension] Option to disable the creation of spans per mojo goal execution

[cyrille-leclerc]

Description:

Option to disable the creation of spans per mojo goal execution

Existing Issue(s):

• [maven-extension] Disable Mojo spans #97

Testing:

Unfortunately I don't know yet how to add integration tests for Maven extensions. To manually test

export OTEL_EXPORTER_OTLP_ENDPOINT="http://localhost:4317"
export OTEL_MAVEN_INSTRUMENTATION_MOJOS_ENABLED=true
mvn verify

# verify spans for modules and spans where created

export OTEL_MAVEN_INSTRUMENTATION_MOJOS_ENABLED=false
mvn verify

# verify spans spans where NOT created, only spans for modules where created

Documentation:

Documented the new configuration parameter OTEL_MAVEN_INSTRUMENTATION_MOJOS_ENABLED=true/false / -Dotel.maven.instrumentation.mojos.enabled=true/false

Outstanding items:

Adding integration tests. I plan to engage with the Maven community to get guidance on this.

[cyrille-leclerc]

@kenfinnigan please look at this proposal to solve the problem you raised to disable the creation of spans per mojo goal execution for large builds.

I introduced the configuration parameter OTEL_MAVEN_INSTRUMENTATION_MOJOS_ENABLED=true/false / -Dotel.maven.instrumentation.mojos.enabled=true/false.
The naming convention is aligned on the configuration params to disable instrumentations on the Java agent:

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/suppressing-instrumentation.md

You can suppress agent instrumentation of specific libraries by using
-Dotel.instrumentation.[name].enabled=false (or using the equivalent environment
variable OTEL_INSTRUMENTATION_[NAME]_ENABLED) where name (NAME) is the
corresponding instrumentation name

[kenfinnigan]

The description confused me as it implies disabling, but based on the code I think it's meant to be enabling.

Is that correct?

[cyrille-leclerc]

I think that the proposed code keeps mojo spans enabled by default (see L171 and code snippet below).

I'm wondering if the naming convention of to disable instrumentations on the Otel Java Agent, OTEL_INSTRUMENTATION_[NAME]_ENABLED , is intuitive enough for our use case on the Otel Maven extension.

❓ Would it be more intuitive to suffix the config param by _DISABLED (ie OTEL_MAVEN_INSTRUMENTATION_MOJOS_DISABLED) than to follow the _ENABLED convention of the Otel Java Agent?

Code snippet of the proposed code that keeps mojo spans enabled by default thanks to the default value true:

    this.mojosInstrumentationEnabled =
        Boolean.parseBoolean(
            StringUtils.defaultIfBlank(mojosInstrumentationEnabledAsString, "true"));

[anuraaga]

Following the convention of the instrumentation repo, for better or worse, is probably a good idea. I guess @kenfinnigan's pointing to the comment starting with Disable the creation which makes it seem like this flags disables when it's true. I think this would read fine

"Whether to create spans for each mojo goal execution"

[cyrille-leclerc]

Thanks @anuraaga , I have improved the documentation based on your recommendation.

[cyrille-leclerc]

Thanks @anuraaga for the edits!

[v1v]

Question regarding the naming convention:

Should OTEL_MAVEN_INSTRUMENTATION_MOJOS_ENABLED be renamed to OTEL_INSTRUMENTATION_MAVEN_MOJOS_ENABLED?

Similar to the example for OTEL_INSTRUMENTATION_AKKA_ACTOR_ENABLED in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/suppressing-instrumentation.md#even-more-fine-grained-control ?

I understand it's not purely a java instrumentation by definition, so I don't know if it makes sense to use the same format or whether this might cause some misleading.

Other than that 💯

[kenfinnigan]

Based on the ones I see here: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/config/common.md

I would agree it should probably be OTEL_INSTRUMENTATION_MAVEN_MOJOS_ENABLED

[cyrille-leclerc]

Great catch @v1v , the Maven extension doesn't need to have a different configuration prefix.
I updated to use -Dotel.instrumentation.maven-mojos.enabled and OTEL_INSTRUMENTATION_MAVEN_MOJOS_ENABLED

[kenfinnigan]

Bit of a nitpick, but can it be maven.mojos instead of a hyphen separator?

If additional properties are added in the future it's good to have them all under maven.* and it wouldn't require a rename of this one to do that

[cyrille-leclerc]

No problem, I wanted to highlight the usage of - over . but I forgot to explain the rationale.
I used - rather than . to align with the convention chosen for "libraries and frameworks" in suppressing-instrumentation.md. All libs and frameworks use -, even broad namespaces like spring-* when I didn't find example of libraries and frameworks using ..

I'll switch from maven-* to maven.* if desired.

[kenfinnigan]

I understand the rationale of the - over . based on the linked document, but I wonder if it applies given there isn't a separate library of instrumentation for Maven vs Maven Mojo. I consider it to be a Maven instrumentation and not a Maven Mojo one, meaning maven is the instrumentation and not maven-mojo.

I could easily be splitting hairs on this, so please ignore me if that's the case.

[cyrille-leclerc]

I like you proposal, I pushed a change.

Last verification, I switched to mojo singular rather than plural.

Choosing -Dotel.instrumentation.maven.mojo.enabled and OTEL_INSTRUMENTATION_MAVEN_MOJO_ENABLED to match with the usage of singular on suppressing-instrumentation.md and to match with what you just said in your comment.

Thanks again for your patience.

[cyrille-leclerc]

Thanks @trask!