use upstream span stacktrace #239
Conversation
|
|
||
| # otel contrib | ||
| # updated from upstream agent with .ci/update-upstream.sh | ||
| opentelemetryContribAlpha = "1.34.0-alpha" | ||
| opentelemetryContribAlpha = "1.36.0-alpha" |
There was a problem hiding this comment.
[for reviewer] this is the first version where the contrib artifact is published
| @@ -10,11 +10,11 @@ opentelemetryProto = "1.3.1-alpha" | |||
|
|
|||
| # otel agent, we rely on the '*-alpha' and get the non-alpha dependencies transitively | |||
| # updated from upstream agent with .ci/update-upstream.sh | |||
| opentelemetryJavaagentAlpha = "2.3.0-alpha" | |||
| opentelemetryJavaagentAlpha = "2.4.0-alpha" | |||
There was a problem hiding this comment.
[for reviewer] updating to the latest version allows to also bump the latest SDK and align with the version used for contrib (maybe a further improvement would be to ensure that we don't have any mismatch).
buildSrc/src/main/kotlin/elastic-otel.agent-packaging-conventions.gradle.kts
Outdated
Show resolved
Hide resolved
| @@ -19,6 +19,7 @@ | |||
| package co.elastic.otel.common; | |||
|
|
|||
| import io.opentelemetry.api.trace.Span; | |||
| import io.opentelemetry.contrib.stacktrace.internal.MutableSpan; | |||
There was a problem hiding this comment.
I feel like replacing our MutableSpan from the distro with the copy from contrib is a bad idea:
- In contrib, it is not available as a "public" type in a shared library, it is only an implementation detail of the span-stacktrace processor at the moment. As soon as the SDK allows span processors to modify spans on end, it will be gone, though we will still need it for the profiling correlation
- It will make our live a lot harder maintaining extensions which won't be contributed (yet) upstream: We'll have to go to contrib, update the utility there, wait for a release and then only have the changes available
custom/build.gradle.kts
Outdated
| implementation(project(":resources")) | ||
| compileOnly("io.opentelemetry:opentelemetry-sdk") | ||
| compileOnly("io.opentelemetry:opentelemetry-sdk-extension-autoconfigure-spi") | ||
| compileOnly("io.opentelemetry.javaagent:opentelemetry-javaagent-extension-api") | ||
| compileOnly("io.opentelemetry.javaagent:opentelemetry-javaagent-tooling") | ||
| compileOnly(libs.bundles.semconv) | ||
|
|
||
| implementation(libs.contribSpanStacktrace) |
There was a problem hiding this comment.
Have you tried excluding the transitive dependencies here instead of having to deal with it in the packaging code?
Seems cleaner to me:
https://docs.gradle.org/current/userguide/dependency_downgrade_and_exclude.html#sec:excluding-transitive-deps
There was a problem hiding this comment.
Excluding transitive dependencies is close to use compileOnly instead of implementation: we compile against the API, but don't pull transitive dependencies when used.
The downside of this approach is that we then have to pull back needed dependencies again, for example with tests in this module require an extra testImplementation, and all the modules that depend on custom will have to do the same.
In a sense we need to have the ability to express "use this for implementation, but some transitive dependencies need to be filtered-out in packaging", and I don't know if we can do that with a custom dependency type.
There was a problem hiding this comment.
The downside of this approach is that we then have to pull back needed dependencies again, for example with tests in this module require an extra testImplementation, and all the modules that depend on custom will have to do the same.
My understanding is that custom is not intended to be used by anyone but as an aggregator for building our agent-distro and the extension. both of those don't want to have the transitive dependencies.
For having to pull in the transitive dependencies for tests:
Simply adding testImplementation(libsContribSpanStacktrace) should pull in everything that has been excluded.
Alternatively, the commonly used OpenTelemetry modules for tests could be added to the test classpath via our testing-common module or an convention.
The main downside I see with manually removing the "bad" classes at packaging time is that it makes the build process depend on package names, which are neither public nor guaranteed to be stable (e.g. io/opentelemetry/extension/incubator/), making the build potentially prone to errors which are hard to find.
In my opinion the extension (e.g. span-stacktrace) should not be pulling in SDK dependencies via implementation anyway and use compileOnly , because it doesn't make just our live harder: It also makes the life harder for people who want to package those extensions as their own agent-extension fatJar, becuase they have to do the same exclusions.
For people who use the SDK directly I'd expect them to add the SDK to their classpath explicitly and not rely on transitive dependencies of an SDK-extension, which is kind of the wrong way around.
There was a problem hiding this comment.
This might also be something to discuss with the Otel Java SIG to see if they have any opinion on it. Looking at the contrib repo, most extensions there pull in the SDK + API as api(...), which has the downsides (and IMO no upsides) explained above.
Depends on PR to be merged + released to avoid relying on a local snapshot.
Final step of #172
Fixes #172
Checklist