-
Notifications
You must be signed in to change notification settings - Fork 1k
Peer service testing #14963
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
base: main
Are you sure you want to change the base?
Peer service testing #14963
Conversation
|
🔧 The result from spotlessApply was committed to the PR branch. |
1 similar comment
|
🔧 The result from spotlessApply was committed to the PR branch. |
4d2a8de to
1d59409
Compare
| tasks { | ||
| test { | ||
| systemProperty("collectMetadata", findProperty("collectMetadata")?.toString() ?: "false") | ||
| systemProperty("otel.instrumentation.common.peer-service-mapping", "127.0.0.1=test-peer-service,localhost=test-peer-service,192.0.2.1=test-peer-service") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be simplified by #13434
387f86e to
3378a01
Compare
|
🔧 The result from spotlessApply was committed to the PR branch. |
1 similar comment
|
🔧 The result from spotlessApply was committed to the PR branch. |
| .hasParent(trace.getSpan(0)) | ||
| .hasStatus(StatusData.error()) | ||
| .hasAttributesSatisfyingExactly( | ||
| .hasAttributesSatisfying( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relaxing here was needed because this is shared by both Java agent tests and library tests
and I decided to only test peer.service in Java agent tests (though we could revisit that decision)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add hasPeerService to the super class or expose it through options. That method could return true only when running with the agent. To detect the presence of the agent we could Class.forName something or even check for otel.javaagent.debug that we know is only set for the agent. Instead of hasPeerService might also consider getExpectedPeerServiceName that would return null when running without the agent, that could allow simpler asserts as null is the same as missing attribute.
| .hasKind(CLIENT) | ||
| .hasParent(trace.getSpan(0)) | ||
| .hasAttributesSatisfyingExactly( | ||
| .hasAttributesSatisfying( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same reason here
| }) | ||
| .satisfies( | ||
| spanData -> { | ||
| // Check for peer.service when running with javaagent instrumentation | ||
| String distroName = | ||
| spanData | ||
| .getResource() | ||
| .getAttribute(TelemetryIncubatingAttributes.TELEMETRY_DISTRO_NAME); | ||
| if ("opentelemetry-java-instrumentation".equals(distroName)) { | ||
| String expectedPeerService = options.getExpectedPeerServiceName().apply(uri); | ||
| if (expectedPeerService != null) { | ||
| assertThat(spanData.getAttributes()) | ||
| .containsEntry(PeerIncubatingAttributes.PEER_SERVICE, expectedPeerService); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to deal with configuring all of the library http client telemetry builders, but now that the javaagent ones are working, I don't mind revisiting
(in general I don't think peer.service infra is as important for library instrumentation because it's generally easy to add a peer.service constant identifier when you add instrumentation around a particular service call in that case)
| Builder setTestPeerService(boolean value); | ||
|
|
||
| Builder setExpectedPeerServiceName(Function<URI, String> value); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really used with current hacky implementation, but maybe they should be
| tasks { | ||
| test { | ||
| systemProperty("collectMetadata", findProperty("collectMetadata")?.toString() ?: "false") | ||
| systemProperty("otel.instrumentation.common.peer-service-mapping", "127.0.0.1=test-peer-service,localhost=test-peer-service,192.0.2.1=test-peer-service") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of setting this for every test we could use something like https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/testing/agent-exporter/src/main/java/io/opentelemetry/javaagent/testing/http/CapturedHttpHeadersTestConfigSupplier.java to set it in one place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or in
Line 97 in dbd47db
| "-Dotel.java.experimental.span-attributes.copy-from-baggage.include=test-baggage-key-1,test-baggage-key-2" |
c37ef46 to
5068473
Compare
|
🔧 The result from spotlessApply was committed to the PR branch. |
7a23016 to
8c33824
Compare
8c33824 to
7a717eb
Compare
7a717eb to
7a32b2b
Compare
Library instrumentation doesn't support peer-service-mapping configuration (javaagent-only feature). Removed peer.service expectations from shared test base class AbstractDubboTraceChainTest.java to fix failures with Dubbo 3.3.5.
instrumentation/vertx/vertx-rx-java-3.5/javaagent/build.gradle.kts
Outdated
Show resolved
Hide resolved
instrumentation/spring/spring-web/spring-web-3.1/javaagent/build.gradle.kts
Outdated
Show resolved
Hide resolved
...pentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyConnectionSpanTest.java
Outdated
Show resolved
Hide resolved
...pentelemetry/javaagent/instrumentation/reactornetty/v0_9/ReactorNettyConnectionSpanTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/Netty41ClientSslTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/Netty41ClientSslTest.java
Show resolved
Hide resolved
...test/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_0/LettuceSyncClientTest.java
Outdated
Show resolved
Hide resolved
...est/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_0/LettuceAsyncClientTest.java
Outdated
Show resolved
Hide resolved
...est/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_0/LettuceAsyncClientTest.java
Outdated
Show resolved
Hide resolved
CLIENT spans now include peer.service attribute when peer-service-mapping is configured.
- Remove empty test configuration blocks from build.gradle.kts files - Don't reorder attributes in test assertions - add PEER_SERVICE at end instead - Allow both error message formats in Lettuce tests (with and without 'getsockopt:')
instrumentation/jaxrs-client/jaxrs-client-1.1-testing/build.gradle.kts
Outdated
Show resolved
Hide resolved
...est/java/io/opentelemetry/javaagent/instrumentation/lettuce/v4_0/LettuceAsyncClientTest.java
Outdated
Show resolved
Hide resolved
...est/java/io/opentelemetry/javaagent/instrumentation/lettuce/v4_0/LettuceAsyncClientTest.java
Outdated
Show resolved
Hide resolved
| val -> | ||
| val.isIn( | ||
| "Connection refused: " | ||
| + host | ||
| + "/" | ||
| + ip | ||
| + ":" | ||
| + incorrectPort, | ||
| "Connection refused: getsockopt: " | ||
| + host | ||
| + "/" | ||
| + ip | ||
| + ":" | ||
| + incorrectPort)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change is just due to me running on Windows and getting a different message
...st/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/Netty41ConnectionSpanTest.java
Show resolved
Hide resolved
- Remove empty test configuration blocks from build.gradle.kts files - Delete temporary 'peer' file - Fix attribute ordering in tests: keep original order and add peer.service at end - Lettuce 4.0 LettuceAsyncClientTest - Netty 4.1 ConnectionSpanTest and ClientSslTest - ReactorNetty 0.9 and 1.0 ConnectionSpanTest
…ng-data - Update AbstractHibernateReactiveTest to expect peer.service attribute - Update ReactiveSpringDataTest to expect peer.service attribute - Add stringKey import to both test files
Resolves #10361