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.

Sign up for GitHub

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

Inline incubating attributes + central semconv-incubating dependency #1298

Merged
merged 16 commits into from
May 28, 2024
Merged

Inline incubating attributes + central semconv-incubating dependency #1298

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
58e0e32
use semconv for code.stacktrace
SylvainJuge May 2, 2024
e26e173
central semconv incubating dependency
SylvainJuge May 2, 2024
87a8e4b
spotless
SylvainJuge May 2, 2024
6723814
migrate gcp provider
SylvainJuge May 24, 2024
30b3f81
inline incubating semconv for aws resources
SylvainJuge May 24, 2024
a78fea3
migrate last bits
SylvainJuge May 24, 2024
f07378d
add comment for clarification
SylvainJuge May 24, 2024
a3b01cf
simplify simple cases
SylvainJuge May 24, 2024
a24fd04
add comment
SylvainJuge May 24, 2024
491f349
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-c…
SylvainJuge May 24, 2024
69872b3
migrate maven extension
SylvainJuge May 24, 2024
1886937
add explicit reference to non-incubating semconv
SylvainJuge May 28, 2024
67fc874
rename to semConvVersion
SylvainJuge May 28, 2024
2ec01c6
Merge remote-tracking branch 'upstream/main' into span-stacktrace-sem…
trask May 28, 2024
4da5808
Fix merge
trask May 28, 2024
891de38
spotless
trask May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion aws-resources/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ dependencies {
api("io.opentelemetry:opentelemetry-sdk")

implementation("io.opentelemetry.semconv:opentelemetry-semconv")
implementation("io.opentelemetry.semconv:opentelemetry-semconv-incubating:1.25.0-alpha")
implementation("io.opentelemetry.semconv:opentelemetry-semconv-incubating")

compileOnly("io.opentelemetry:opentelemetry-sdk-extension-autoconfigure")

Expand Down
2 changes: 2 additions & 0 deletions dependencyManagement/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ val errorProneVersion = "2.26.1"
val prometheusVersion = "0.16.0"
val mockitoVersion = "4.11.0"
val slf4jVersion = "2.0.13"
val contribIncubatingVersion = "1.25.0-alpha"
SylvainJuge marked this conversation as resolved.
Show resolved Hide resolved

val CORE_DEPENDENCIES = listOf(
"com.google.auto.service:auto-service:${autoServiceVersion}",
Expand All @@ -33,6 +34,7 @@ val CORE_DEPENDENCIES = listOf(
"com.google.errorprone:error_prone_core:${errorProneVersion}",
"io.github.netmikey.logunit:logunit-jul:2.0.0",
"io.opentelemetry.proto:opentelemetry-proto:1.0.0-alpha",
"io.opentelemetry.semconv:opentelemetry-semconv-incubating:${contribIncubatingVersion}",
SylvainJuge marked this conversation as resolved.
Show resolved Hide resolved
"io.prometheus:simpleclient:${prometheusVersion}",
"io.prometheus:simpleclient_common:${prometheusVersion}",
"io.prometheus:simpleclient_httpserver:${prometheusVersion}",
Expand Down
2 changes: 1 addition & 1 deletion gcp-resources/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ dependencies {
implementation("com.google.cloud.opentelemetry:detector-resources-support:0.28.0")

implementation("io.opentelemetry.semconv:opentelemetry-semconv")
implementation("io.opentelemetry.semconv:opentelemetry-semconv-incubating:1.25.0-alpha")
implementation("io.opentelemetry.semconv:opentelemetry-semconv-incubating")
Copy link
Member

Choose a reason for hiding this comment

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

All these usages of opentelemetry-semconv-incubating go against our stated advice:

Library instrumentation SHOULD NOT depend on this.

Libraries can use this for testing, but should make copies of the attributes to avoid possible runtime errors from version conflicts.

I think we should follow our own advice to avoid unresolvable transitive version conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder, I completely missed it.

I'm definitely missing the context on the intent of this advice, so maybe there is something to clarify here.

For now, the way I understand things:

The opentelemetry-semconv artifact only contains stable semantic conventions.
The opentelemetry-semconv-incubating artifact only contains incubating semantic conventions.

Consumers of semantic conventions who need/want to avoid breaking change:

  • should have a direct dependency on opentelemetry-semconv.
  • might have a test dependency on opentelemetry-semconv-incubating.
  • should inline the incubating semconv attributes in the production code, this is where they trade maintainability for stability.

Consumers of semantic conventions who can tolerate/handle breaking changes:

  • should have a direct dependency on opentelemetry-semconv
  • should have a direct dependency on opentelemetry-semconv-incubating which provides an easy way to find usages and refactor in the IDE.

For example:

  • in the instrumentation agent, we already use the incubating artifact and can embrace breaking changes quite easily due to the large set of contributors who can help. However, the goal is to have as much stable as possible and sometime the incubating metrics (like the JVM metrics) are opt-in through configuration, however this option is not always possible at attribute level.
  • for a module in contrib that has only two contributors and is rarely updated, keeping it stable is probably a better option, so having some duplication is a good compromise.

To me, by adding a dependency on opentelemetry-semconv-incubating on a module in contrib, you opt-in for some breaking changes.

Maybe here having a single version used for all the contrib modules is a bold move and having each module handle its own version (even if they are currently aligned) is a better compromise, so each module can be updated at its own pace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a bit of research I found that it comes from open-telemetry/semantic-conventions-java#41, and open-telemetry/semantic-conventions-java#22 (comment) provides some background on the issue with transitive dependencies.

However, this is something that we need to solve in a separate issue/PR, and maybe implementing a "per version namespace for the non-stable attributes" as suggested here in https://github.com/open-telemetry/semantic-conventions-java/ could help.

From what I understand the proposal is to have the generated semconv classes with a dedicated package per version (for example io.opentelemetry.semconv.v1_25_0) and to only keep a few older versions in the incubating artifact to prevent breaking existing usages.


compileOnly("io.opentelemetry:opentelemetry-sdk-extension-autoconfigure")

Expand Down
2 changes: 1 addition & 1 deletion samplers/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ otelJava.moduleName.set("io.opentelemetry.contrib.sampler")
dependencies {
api("io.opentelemetry:opentelemetry-sdk")
implementation("io.opentelemetry.semconv:opentelemetry-semconv")
implementation("io.opentelemetry.semconv:opentelemetry-semconv-incubating:1.25.0-alpha")
implementation("io.opentelemetry.semconv:opentelemetry-semconv-incubating")
testImplementation("io.opentelemetry:opentelemetry-sdk-extension-autoconfigure")
api("io.opentelemetry:opentelemetry-sdk-extension-autoconfigure-spi")
}
2 changes: 2 additions & 0 deletions span-stacktrace/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,6 @@ description = "OpenTelemetry Java span stacktrace capture module"
dependencies {
api("io.opentelemetry:opentelemetry-sdk")
testImplementation("io.opentelemetry:opentelemetry-sdk-testing")

implementation("io.opentelemetry.semconv:opentelemetry-semconv-incubating")
}
9 changes: 3 additions & 6 deletions ...stacktrace/src/main/java/io/opentelemetry/contrib/stacktrace/StackTraceSpanProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@

package io.opentelemetry.contrib.stacktrace;

import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.contrib.stacktrace.internal.AbstractSimpleChainingSpanProcessor;
import io.opentelemetry.contrib.stacktrace.internal.MutableSpan;
import io.opentelemetry.sdk.trace.ReadableSpan;
import io.opentelemetry.sdk.trace.SpanProcessor;
import io.opentelemetry.semconv.incubating.CodeIncubatingAttributes;
import java.io.PrintWriter;
import java.io.StringWriter;
import java.util.function.Predicate;
Expand All @@ -18,9 +18,6 @@

public class StackTraceSpanProcessor extends AbstractSimpleChainingSpanProcessor {

// TODO : remove this once semconv 1.24.0 is available
static final AttributeKey<String> SPAN_STACKTRACE = AttributeKey.stringKey("code.stacktrace");

private static final Logger logger = Logger.getLogger(StackTraceSpanProcessor.class.getName());

private final long minSpanDurationNanos;
Expand Down Expand Up @@ -58,7 +55,7 @@ protected ReadableSpan doOnEnd(ReadableSpan span) {
if (span.getLatencyNanos() < minSpanDurationNanos) {
return span;
}
if (span.getAttribute(SPAN_STACKTRACE) != null) {
if (span.getAttribute(CodeIncubatingAttributes.CODE_STACKTRACE) != null) {
// Span already has a stacktrace, do not override
return span;
}
Expand All @@ -68,7 +65,7 @@ protected ReadableSpan doOnEnd(ReadableSpan span) {
MutableSpan mutableSpan = MutableSpan.makeMutable(span);

String stacktrace = generateSpanEndStacktrace();
mutableSpan.setAttribute(SPAN_STACKTRACE, stacktrace);
mutableSpan.setAttribute(CodeIncubatingAttributes.CODE_STACKTRACE, stacktrace);
return mutableSpan;
}

Expand Down
5 changes: 3 additions & 2 deletions ...ktrace/src/test/java/io/opentelemetry/contrib/stacktrace/StackTraceSpanProcessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import io.opentelemetry.sdk.trace.SpanProcessor;
import io.opentelemetry.sdk.trace.data.SpanData;
import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor;
import io.opentelemetry.semconv.incubating.CodeIncubatingAttributes;
import java.time.Instant;
import java.util.List;
import java.util.function.Consumer;
Expand Down Expand Up @@ -54,7 +55,7 @@ void spanWithExistingStackTrace() {
span -> true,
20,
1,
sb -> sb.setAttribute(StackTraceSpanProcessor.SPAN_STACKTRACE, "hello"),
sb -> sb.setAttribute(CodeIncubatingAttributes.CODE_STACKTRACE, "hello"),
stacktrace -> assertThat(stacktrace).isEqualTo("hello"));
}

Expand Down Expand Up @@ -99,7 +100,7 @@ private void testSpan(

if (!finishedSpans.isEmpty()) {
String stackTrace =
finishedSpans.get(0).getAttributes().get(StackTraceSpanProcessor.SPAN_STACKTRACE);
finishedSpans.get(0).getAttributes().get(CodeIncubatingAttributes.CODE_STACKTRACE);

stackTraceCheck.accept(stackTrace);
}
Expand Down
Loading