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

Jump to bottom

Update dependency io.zipkin.reporter2:zipkin-reporter-bom to v3.2.1 #6151

Merged
merged 2 commits into from
Jan 24, 2024
Merged

Update dependency io.zipkin.reporter2:zipkin-reporter-bom to v3.2.1 #6151

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
738da03
Update dependency io.zipkin.reporter2:zipkin-reporter-bom to v3.2.1
Jan 14, 2024
8383003
Merge branch 'main' into new-sender
jack-berg Jan 23, 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 dependencyManagement/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ val DEPENDENCY_BOMS = listOf(
"com.squareup.okio:okio-bom:3.7.0", // applies to transitive dependencies of okhttp
"io.grpc:grpc-bom:1.60.1",
"io.netty:netty-bom:4.1.104.Final",
"io.zipkin.reporter2:zipkin-reporter-bom:3.1.1",
"io.zipkin.reporter2:zipkin-reporter-bom:3.2.1",
"io.zipkin.brave:brave-bom:5.17.0",
"org.assertj:assertj-bom:3.25.1",
"org.junit:junit-bom:5.10.1",
Expand Down
3 changes: 3 additions & 0 deletions docs/apidiffs/current_vs_latest/opentelemetry-exporter-zipkin.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,6 @@ Comparing source compatibility of against
=== UNCHANGED METHOD: PUBLIC io.opentelemetry.exporter.zipkin.ZipkinSpanExporterBuilder setEncoder(zipkin2.codec.BytesEncoder<zipkin2.Span><zipkin2.Span>)
+++ NEW ANNOTATION: java.lang.Deprecated
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.exporter.zipkin.ZipkinSpanExporterBuilder setEncoder(zipkin2.reporter.BytesEncoder<zipkin2.Span>)
=== UNCHANGED METHOD: PUBLIC io.opentelemetry.exporter.zipkin.ZipkinSpanExporterBuilder setSender(zipkin2.reporter.Sender)
+++ NEW ANNOTATION: java.lang.Deprecated
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.exporter.zipkin.ZipkinSpanExporterBuilder setSender(zipkin2.reporter.BytesMessageSender)
35 changes: 12 additions & 23 deletions exporters/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,8 @@
import java.util.logging.Logger;
import zipkin2.Span;
import zipkin2.reporter.BytesEncoder;
import zipkin2.reporter.Callback;
import zipkin2.reporter.BytesMessageSender;
import zipkin2.reporter.Encoding;
import zipkin2.reporter.Sender;

/**
* This class was based on the <a
Expand All @@ -40,15 +39,15 @@ public final class ZipkinSpanExporter implements SpanExporter {
private final AtomicBoolean isShutdown = new AtomicBoolean();
private final ZipkinSpanExporterBuilder builder;
private final BytesEncoder<Span> encoder;
private final Sender sender;
private final BytesMessageSender sender;
private final ExporterMetrics exporterMetrics;

private final OtelToZipkinSpanTransformer transformer;

ZipkinSpanExporter(
ZipkinSpanExporterBuilder builder,
BytesEncoder<Span> encoder,
Sender sender,
BytesMessageSender sender,
Supplier<MeterProvider> meterProviderSupplier,
OtelToZipkinSpanTransformer transformer) {
this.builder = builder;
Expand Down Expand Up @@ -76,25 +75,15 @@ public CompletableResultCode export(Collection<SpanData> spanDataList) {
encodedSpans.add(encoder.encode(zipkinSpan));
}

CompletableResultCode result = new CompletableResultCode();
sender
.sendSpans(encodedSpans)
.enqueue(
new Callback<Void>() {
@Override
public void onSuccess(Void value) {
exporterMetrics.addSuccess(numItems);
result.succeed();
}

@Override
public void onError(Throwable t) {
exporterMetrics.addFailed(numItems);
logger.log(Level.WARNING, "Failed to export spans", t);
result.fail();
}
});
return result;
try {
sender.send(encodedSpans);
Copy link
Member

Choose a reason for hiding this comment

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

I believe this changes the implementation from being non-blocking to blocking. In practice, this is fine because BatchSpanProcess / SImpleSpanProcessor never calls SpanExporter#export concurrently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep it does make it explicitly blocking (depending on sender some would have sneakily blocked!) Thanks for the notes on impact

exporterMetrics.addSuccess(numItems);
return CompletableResultCode.ofSuccess();
} catch (IOException | RuntimeException t) {
exporterMetrics.addFailed(numItems);
logger.log(Level.WARNING, "Failed to export spans", t);
return CompletableResultCode.ofFailure();
}
}

@Override
Expand Down
33 changes: 24 additions & 9 deletions ...ters/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,15 @@
import javax.annotation.Nullable;
import zipkin2.Span;
import zipkin2.reporter.BytesEncoder;
import zipkin2.reporter.Sender;
import zipkin2.reporter.BytesMessageSender;
import zipkin2.reporter.SpanBytesEncoder;
import zipkin2.reporter.okhttp3.OkHttpSender;

/** Builder class for {@link ZipkinSpanExporter}. */
public final class ZipkinSpanExporterBuilder {
private BytesEncoder<Span> encoder = SpanBytesEncoder.JSON_V2;
private Supplier<InetAddress> localIpAddressSupplier = LocalInetAddressSupplier.getInstance();
@Nullable private Sender sender;
@Nullable private BytesMessageSender sender;
private String endpoint = ZipkinSpanExporter.DEFAULT_ENDPOINT;
// compression is enabled by default, because this is the default of OkHttpSender,
// which is created when no custom sender is set (see OkHttpSender.Builder)
Expand All @@ -38,20 +38,35 @@
* Sets the Zipkin sender. Implements the client side of the span transport. An {@link
* OkHttpSender} is a good default.
*
* <p>The {@link Sender#close()} method will be called when the exporter is shut down.
* <p>The {@link BytesMessageSender#close()} method will be called when the exporter is shut down.
*
* @param sender the Zipkin sender implementation.
* @return this.
* @deprecated Use {@link #setSender(BytesMessageSender)} insteead.
*/
public ZipkinSpanExporterBuilder setSender(Sender sender) {
@Deprecated
public ZipkinSpanExporterBuilder setSender(zipkin2.reporter.Sender sender) {
Copy link
Member

@jack-berg jack-berg Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The javadoc says that this will be removed in version 4. @open-telemetry/java-approvers we should figure out a plan with how we would like to handle this. If we bump to the next major version of zipkin, we wouldn't be able to support this API without a breaking change.

Might need to start publishing zipkin exporter artifacts which encode the major version of the zipkin dependency in the artifact name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW I'm also not sure when we would do a version 4, but the point remains. The upgrade analysis forced this point, but I think critical thinking could make it easier.

Reporter 3.x isn't compatible with 2.x senders anyway. You haven't yet released the 3.x code yet.... one could argue you could remove this deprecated thing now and then not have to worry about v4.

Copy link
Contributor Author

@codefromthecrypt codefromthecrypt Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

for context micrometer and spring boot are waiting for 1.35.0, and that's the first version that has any of the reporter 3 stuff. They will switch all the senders to the floor reporter version in 1.35.0 (which would be 3.2.1 when this lands). I know you have other concerns besides spring, but it is a data point that may help you decide.

Copy link
Member

Choose a reason for hiding this comment

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

You haven't yet released the 3.x code yet.... one could argue you could remove this deprecated thing now and then not have to worry about v4.

Its a good point. We would have to do the same strategy I'm proposing - publish a new artifact id which includes zipkin 3 in the name some how. Deprecate, and eventually stop publishing the existing zipkin exporter artifact.

Added a topic to discuss in the tomorrow's java SIG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep another choice for discussion is to shade it and just expose it as configuration instead of a library hook. anyway thanks for the help!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jack-berg do you have an idea which way you are going to go? If this doesn't merge it isn't dire, just I will need to help split micrometer's classpath as the only place this causes a conflict in is tests. I mean it will be a conflict in real life, too, but only for those simultaneously using otel and also direct zipkin stuff.

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 inclined to merge this PR and continue to publish the artifact under the coordinates. Then, we can consider publishing new artifact coordinates when zipkin publishes a v4 with the breaking change. Solving the problem now seems like putting the cart ahead of the horse.

@open-telemetry/java-approvers if there are no dissenting additional thoughts on this I'll plan on merging on 1/24/24.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jack-berg sounds good to me, and helps others from having more supporting work than maybe they can budget. v4 was just in the javadoc to tell people to stop using the old stuff as I noticed the senders were used outside of the reporters in that project. I made an issue about v4 and to not do that until it is important, likely with a new groupId/package openzipkin/zipkin-reporter-java#247

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 inclined to merge this PR and continue to publish the artifact under the coordinates. Then, we can consider publishing new artifact coordinates when zipkin publishes a v4 with the breaking change. Solving the problem now seems like putting the cart ahead of the horse.

👍

return setSender((BytesMessageSender) sender);

Check warning on line 49 in exporters/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterBuilder.java

View check run for this annotation

Codecov / codecov/patch

exporters/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterBuilder.java#L49 

Added line #L49 was not covered by tests
}

/**
* Sets the Zipkin sender. Implements the client side of the span transport. An {@link
* OkHttpSender} is a good default.
*
* <p>The {@link BytesMessageSender#close()} method will be called when the exporter is shut down.
*
* @param sender the Zipkin sender implementation.
* @return this.
*/
public ZipkinSpanExporterBuilder setSender(BytesMessageSender sender) {
requireNonNull(sender, "sender");
this.sender = sender;
return this;
}

/**
* Sets the {@link zipkin2.codec.BytesEncoder}, which controls the format used by the {@link
* Sender}. Defaults to the {@link zipkin2.codec.SpanBytesEncoder#JSON_V2}.
* BytesMessageSender}. Defaults to the {@link zipkin2.codec.SpanBytesEncoder#JSON_V2}.
*
* @param encoder the {@code BytesEncoder} to use.
* @return this.
Expand All @@ -65,8 +80,8 @@
}

/**
* Sets the {@link BytesEncoder}, which controls the format used by the {@link Sender}. Defaults
* to the {@link SpanBytesEncoder#JSON_V2}.
* Sets the {@link BytesEncoder}, which controls the format used by the {@link
* BytesMessageSender}. Defaults to the {@link SpanBytesEncoder#JSON_V2}.
*
* @param encoder the {@code BytesEncoder} to use.
* @return this.
Expand Down Expand Up @@ -114,7 +129,7 @@
* supported compression methods include "gzip" and "none".
*
* <p>The compression method is ignored when a custom Zipkin sender is set via {@link
* #setSender(Sender)}.
* #setSender(BytesMessageSender)}.
*
* @param compressionMethod The compression method, ex. "gzip".
* @return this.
Expand Down Expand Up @@ -189,7 +204,7 @@
* @return a {@code ZipkinSpanExporter}.
*/
public ZipkinSpanExporter build() {
Sender sender = this.sender;
BytesMessageSender sender = this.sender;
if (sender == null) {
sender =
OkHttpSender.newBuilder()
Expand Down
5 changes: 4 additions & 1 deletion ...in/src/test/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterEndToEndHttpTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import zipkin2.Endpoint;
import zipkin2.Span;
import zipkin2.codec.SpanBytesDecoder;
import zipkin2.reporter.BytesMessageSender;
import zipkin2.reporter.Encoding;
import zipkin2.reporter.SpanBytesEncoder;
import zipkin2.reporter.okhttp3.OkHttpSender;
Expand Down Expand Up @@ -175,8 +176,10 @@ void testExportFailedAsWrongEncoderUsed() {

private static ZipkinSpanExporter buildZipkinExporter(
String endpoint, Encoding encoding, SpanBytesEncoder encoder, MeterProvider meterProvider) {
BytesMessageSender sender =
OkHttpSender.newBuilder().endpoint(endpoint).encoding(encoding).build();
return ZipkinSpanExporter.builder()
.setSender(OkHttpSender.newBuilder().endpoint(endpoint).encoding(encoding).build())
.setSender(sender)
.setEncoder(encoder)
.setMeterProvider(meterProvider)
.setLocalIpAddressSupplier(() -> localIp)
Expand Down
37 changes: 10 additions & 27 deletions exporters/zipkin/src/test/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@
import static io.opentelemetry.exporter.zipkin.ZipkinTestUtil.zipkinSpanBuilder;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

Expand All @@ -31,26 +30,23 @@
import org.mockito.junit.jupiter.MockitoExtension;
import zipkin2.Span;
import zipkin2.reporter.BytesEncoder;
import zipkin2.reporter.Call;
import zipkin2.reporter.Callback;
import zipkin2.reporter.BytesMessageSender;
import zipkin2.reporter.Encoding;
import zipkin2.reporter.Sender;
import zipkin2.reporter.SpanBytesEncoder;

@ExtendWith(MockitoExtension.class)
class ZipkinSpanExporterTest {

@Mock private Sender mockSender;
@Mock private BytesMessageSender mockSender;
@Mock private SpanBytesEncoder mockEncoder;
@Mock private Call<Void> mockZipkinCall;
@Mock private OtelToZipkinSpanTransformer mockTransformer;
@Mock private InetAddress localIp;

@RegisterExtension
LogCapturer logs = LogCapturer.create().captureForType(ZipkinSpanExporter.class);

@Test
void testExport() {
void testExport() throws IOException {
TestSpanData testSpanData = spanBuilder().build();

ZipkinSpanExporter zipkinSpanExporter =
Expand All @@ -68,25 +64,18 @@ void testExport() {
.build();
when(mockTransformer.generateSpan(testSpanData)).thenReturn(zipkinSpan);
when(mockEncoder.encode(zipkinSpan)).thenReturn(someBytes);
when(mockSender.sendSpans(Collections.singletonList(someBytes))).thenReturn(mockZipkinCall);
doAnswer(
invocation -> {
Callback<Void> callback = invocation.getArgument(0);
callback.onSuccess(null);
return null;
})
.when(mockZipkinCall)
.enqueue(any());

CompletableResultCode resultCode =
zipkinSpanExporter.export(Collections.singleton(testSpanData));

assertThat(resultCode.isSuccess()).isTrue();

verify(mockSender).send(Collections.singletonList(someBytes));
}

@Test
@SuppressLogger(ZipkinSpanExporter.class)
void testExport_failed() {
void testExport_failed() throws IOException {
TestSpanData testSpanData = spanBuilder().build();

ZipkinSpanExporter zipkinSpanExporter =
Expand All @@ -104,20 +93,14 @@ void testExport_failed() {
.build();
when(mockTransformer.generateSpan(testSpanData)).thenReturn(zipkinSpan);
when(mockEncoder.encode(zipkinSpan)).thenReturn(someBytes);
when(mockSender.sendSpans(Collections.singletonList(someBytes))).thenReturn(mockZipkinCall);
doAnswer(
invocation -> {
Callback<Void> callback = invocation.getArgument(0);
callback.onError(new IOException());
return null;
})
.when(mockZipkinCall)
.enqueue(any());
doThrow(new IOException()).when(mockSender).send(Collections.singletonList(someBytes));

CompletableResultCode resultCode =
zipkinSpanExporter.export(Collections.singleton(testSpanData));

assertThat(resultCode.isSuccess()).isFalse();

verify(mockSender).send(Collections.singletonList(someBytes));
}

@Test
Expand Down
Loading