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

Populate Zipkin remoteEndpoint #4933

Merged
merged 5 commits into from
Nov 28, 2022
Merged

Populate Zipkin remoteEndpoint #4933

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5855a24
Populate Zipkin remoteEndpoint
jonatan-ivanov Nov 10, 2022
cddd6a6
Add conditions for creating zipkin remote endpoint
jonatan-ivanov Nov 15, 2022
53b04a1
Merge branch 'main' into zipkin-remote-endpoint
jonatan-ivanov Nov 15, 2022
9eb08c2
Parameterize remote endpoint tests with span kind
jonatan-ivanov Nov 16, 2022
505a7da
Verify INTERNAL span kind too
jonatan-ivanov Nov 18, 2022
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
26 changes: 22 additions & 4 deletions ...rs/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/OtelToZipkinSpanTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
package io.opentelemetry.exporter.zipkin;

import static io.opentelemetry.api.common.AttributeKey.stringKey;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NET_PEER_PORT;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NET_SOCK_PEER_ADDR;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.PEER_SERVICE;
import static java.util.concurrent.TimeUnit.NANOSECONDS;

import io.opentelemetry.api.common.AttributeKey;
Expand Down Expand Up @@ -72,8 +75,6 @@ private OtelToZipkinSpanTransformer(Supplier<InetAddress> ipAddressSupplier) {
* @return a new Zipkin Span
*/
Span generateSpan(SpanData spanData) {
Endpoint endpoint = getEndpoint(spanData);

long startTimestamp = toEpochMicros(spanData.getStartEpochNanos());
long endTimestamp = toEpochMicros(spanData.getEndEpochNanos());

Expand All @@ -85,7 +86,8 @@ Span generateSpan(SpanData spanData) {
.name(spanData.getName())
.timestamp(toEpochMicros(spanData.getStartEpochNanos()))
.duration(Math.max(1, endTimestamp - startTimestamp))
.localEndpoint(endpoint);
.localEndpoint(getLocalEndpoint(spanData))
.remoteEndpoint(getRemoteEndpoint(spanData));
jonatan-ivanov marked this conversation as resolved.
Show resolved Hide resolved

if (spanData.getParentSpanContext().isValid()) {
spanBuilder.parentId(spanData.getParentSpanId());
Expand Down Expand Up @@ -140,7 +142,7 @@ private static String nullToEmpty(String value) {
return value != null ? value : "";
}

private Endpoint getEndpoint(SpanData spanData) {
private Endpoint getLocalEndpoint(SpanData spanData) {
Attributes resourceAttributes = spanData.getResource().getAttributes();

Endpoint.Builder endpoint = Endpoint.newBuilder();
Expand All @@ -158,6 +160,22 @@ private Endpoint getEndpoint(SpanData spanData) {
return endpoint.build();
}

private static Endpoint getRemoteEndpoint(SpanData spanData) {
// TODO: Implement fallback mechanism:
// https://opentelemetry.io/docs/reference/specification/trace/sdk_exporters/zipkin/#otlp---zipkin
Attributes attributes = spanData.getAttributes();

Endpoint.Builder endpoint = Endpoint.newBuilder();
endpoint.serviceName(attributes.get(PEER_SERVICE));
endpoint.ip(attributes.get(NET_SOCK_PEER_ADDR));
Long port = attributes.get(NET_PEER_PORT);
if (port != null) {
endpoint.port(port.intValue());
}

return endpoint.build();
}

@Nullable
private static Span.Kind toSpanKind(SpanData spanData) {
switch (spanData.getKind()) {
Expand Down
104 changes: 100 additions & 4 deletions ...ipkin/src/test/java/io/opentelemetry/exporter/zipkin/OtelToZipkinSpanTransformerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
import static io.opentelemetry.exporter.zipkin.ZipkinTestUtil.spanBuilder;
import static io.opentelemetry.exporter.zipkin.ZipkinTestUtil.zipkinSpan;
import static io.opentelemetry.exporter.zipkin.ZipkinTestUtil.zipkinSpanBuilder;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NET_PEER_PORT;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NET_SOCK_PEER_ADDR;
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.PEER_SERVICE;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;

Expand Down Expand Up @@ -135,11 +138,11 @@ void generateSpan_ResourceServiceNameMapping() {
Resource.create(Attributes.of(ResourceAttributes.SERVICE_NAME, "super-zipkin-service"));
SpanData data = spanBuilder().setResource(resource).build();

Endpoint expectedEndpoint =
Endpoint expectedLocalEndpoint =
Endpoint.newBuilder().serviceName("super-zipkin-service").ip(localIp).build();
Span expectedZipkinSpan =
zipkinSpan(Span.Kind.SERVER, localIp).toBuilder()
.localEndpoint(expectedEndpoint)
.localEndpoint(expectedLocalEndpoint)
.putTag(OtelToZipkinSpanTransformer.OTEL_STATUS_CODE, "OK")
.build();
assertThat(transformer.generateSpan(data)).isEqualTo(expectedZipkinSpan);
Expand All @@ -149,19 +152,112 @@ void generateSpan_ResourceServiceNameMapping() {
void generateSpan_defaultResourceServiceName() {
SpanData data = spanBuilder().setResource(Resource.empty()).build();

Endpoint expectedEndpoint =
Endpoint expectedLocalEndpoint =
Endpoint.newBuilder()
.serviceName(Resource.getDefault().getAttribute(ResourceAttributes.SERVICE_NAME))
.ip(localIp)
.build();
Span expectedZipkinSpan =
zipkinSpan(Span.Kind.SERVER, localIp).toBuilder()
.localEndpoint(expectedEndpoint)
.localEndpoint(expectedLocalEndpoint)
.putTag(OtelToZipkinSpanTransformer.OTEL_STATUS_CODE, "OK")
.build();
assertThat(transformer.generateSpan(data)).isEqualTo(expectedZipkinSpan);
}

@Test
jonatan-ivanov marked this conversation as resolved.
Show resolved Hide resolved
void generateSpan_RemoteEndpointMapping() {
Attributes attributes =
Attributes.builder()
.put(PEER_SERVICE, "remote-test-service")
.put(NET_SOCK_PEER_ADDR, "8.8.8.8")
.put(NET_PEER_PORT, 42L)
.build();

SpanData spanData =
spanBuilder().setResource(Resource.empty()).setAttributes(attributes).build();

Endpoint expectedLocalEndpoint =
Endpoint.newBuilder()
.serviceName(Resource.getDefault().getAttribute(ResourceAttributes.SERVICE_NAME))
.ip(localIp)
.build();

Endpoint expectedRemoteEndpoint =
Endpoint.newBuilder().serviceName("remote-test-service").ip("8.8.8.8").port(42).build();

Span expectedSpan =
zipkinSpan(Span.Kind.SERVER, localIp).toBuilder()
.localEndpoint(expectedLocalEndpoint)
.remoteEndpoint(expectedRemoteEndpoint)
.putTag(PEER_SERVICE.getKey(), "remote-test-service")
.putTag(NET_SOCK_PEER_ADDR.getKey(), "8.8.8.8")
.putTag(NET_PEER_PORT.getKey(), "42")
Comment on lines +202 to +205
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bummer that these are kinda redundant (duplicated) now -- they exist both in the remoteEndpoint and in the tags. Should we filter these specific keys out of the tags when mapping the attributes over?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fyi: there will be lots of things to filter out: https://opentelemetry.io/docs/reference/specification/trace/sdk_exporters/zipkin/#otlp---zipkin

What happens in this class seems wrong to me (but I guess now it's too late for that); I think the span (SpanData?) should have these as separate fields not as attributes. I'm biased though: I prefer having type-safe fields instead of pouring everything into a "hashmap".

Also, just an fyi: as a user of OTel, I would consider this as a breaking change (a tag will be removed from the span). I'm not sure if doing such a change is even allowed here (maybe it is since semconv is unstable so deleting/renaming attributes and breaking users that way is possible at this point here).
It might worth to check other exporters too, i.e.: will those tags be shipped to Jaeger/OTLP/etc?

Because of these, I think deleting (filtering out) those attributes should happen in a separate issue/pr.

Copy link
Member

Choose a reason for hiding this comment

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

I'm biased though: I prefer having type-safe fields instead of pouring everything into a "hashmap".

I assume the decision to model fields in map-like structure instead of in top level fields is because it affords more ability to experiment without the consequences of adding a top level field we later wish to remove / rename / or change. This relieves bikeshedding (to some extent anyway 😄).

Additionally, since spans can potentially be used to model many many domains, the top level fields could easily get bogged down as more and more top level fields were added.

Copy link
Member Author

Choose a reason for hiding this comment

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

without the consequences of adding a top level field we later wish to remove / rename / or change

I think I see the intent but doing this will also give a false sense of not introducing a breaking change when this happens. Indeed, source and binary compatibility are kept but unfortunately it is still a breaking change since behavioral compatibility is not.

I totally agree with not adding fields for everything (that's what attributes are for) but I also think that this is such a fundamental field that it should be there (like the name of the span or the localEndpoint).

I just wanted to call these out, they don't really belong to the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a "fundamental" field for one type of span, but certainly not universally applicable, which is why it shouldn't be a top level field on the Span, or the SpanData.

Copy link
Member Author

@jonatan-ivanov jonatan-ivanov Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I guess that's why it should be optional/@Nullable but to me that does not make it less important.
The fact that it is optional/not universally applicable does not justify "stringly" typing to me; if something is optional, it usually does not go into a hashmap.
I guess this is true for other parts of the OTel API/SDK, there are things that are optional but they have their own fields and they are not stringy typed, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're arguing with the wrong crowd here... this would be something that would need to be added in the specification, and not something we would or could unilaterally add to the java APIs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yepp, that's why I wrote that I think now it's too late for that.

.putTag(OtelToZipkinSpanTransformer.OTEL_STATUS_CODE, "OK")
.build();

assertThat(transformer.generateSpan(spanData)).isEqualTo(expectedSpan);
}

@Test
jonatan-ivanov marked this conversation as resolved.
Show resolved Hide resolved
void generateSpan_RemoteEndpointMappingWhenPortIsMissing() {
Attributes attributes =
Attributes.builder()
.put(PEER_SERVICE, "remote-test-service")
.put(NET_SOCK_PEER_ADDR, "8.8.8.8")
.build();

SpanData spanData =
spanBuilder().setResource(Resource.empty()).setAttributes(attributes).build();

Endpoint expectedLocalEndpoint =
Endpoint.newBuilder()
.serviceName(Resource.getDefault().getAttribute(ResourceAttributes.SERVICE_NAME))
.ip(localIp)
.build();

Endpoint expectedRemoteEndpoint =
Endpoint.newBuilder().serviceName("remote-test-service").ip("8.8.8.8").build();

Span expectedSpan =
zipkinSpan(Span.Kind.SERVER, localIp).toBuilder()
.localEndpoint(expectedLocalEndpoint)
.remoteEndpoint(expectedRemoteEndpoint)
.putTag(PEER_SERVICE.getKey(), "remote-test-service")
.putTag(NET_SOCK_PEER_ADDR.getKey(), "8.8.8.8")
.putTag(OtelToZipkinSpanTransformer.OTEL_STATUS_CODE, "OK")
.build();

assertThat(transformer.generateSpan(spanData)).isEqualTo(expectedSpan);
}

@Test
void generateSpan_RemoteEndpointMappingWhenIpAndPortAreMissing() {
Attributes attributes = Attributes.builder().put(PEER_SERVICE, "remote-test-service").build();

SpanData spanData =
spanBuilder().setResource(Resource.empty()).setAttributes(attributes).build();

Endpoint expectedLocalEndpoint =
Endpoint.newBuilder()
.serviceName(Resource.getDefault().getAttribute(ResourceAttributes.SERVICE_NAME))
.ip(localIp)
.build();

Endpoint expectedRemoteEndpoint =
Endpoint.newBuilder().serviceName("remote-test-service").build();

Span expectedSpan =
zipkinSpan(Span.Kind.SERVER, localIp).toBuilder()
.localEndpoint(expectedLocalEndpoint)
.remoteEndpoint(expectedRemoteEndpoint)
.putTag(PEER_SERVICE.getKey(), "remote-test-service")
.putTag(OtelToZipkinSpanTransformer.OTEL_STATUS_CODE, "OK")
.build();

assertThat(transformer.generateSpan(spanData)).isEqualTo(expectedSpan);
}

@Test
void generateSpan_WithAttributes() {
Attributes attributes =
Expand Down