-
Notifications
You must be signed in to change notification settings - Fork 850
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
Unify compression configuration for exporters #4775
Unify compression configuration for exporters #4775
Conversation
938b9dd
to
ac8af97
Compare
Codecov ReportBase: 90.79% // Head: 90.92% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #4775 +/- ##
============================================
+ Coverage 90.79% 90.92% +0.13%
+ Complexity 4839 4808 -31
============================================
Files 554 545 -9
Lines 14422 14340 -82
Branches 1402 1383 -19
============================================
- Hits 13094 13039 -55
+ Misses 910 894 -16
+ Partials 418 407 -11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
exporters/jaeger/src/test/java/io/opentelemetry/exporter/jaeger/JaegerGrpcSpanExporterTest.java
Show resolved
Hide resolved
exporters/zipkin/src/test/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterTest.java
Outdated
Show resolved
Hide resolved
...utoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/SpanExporterConfiguration.java
Outdated
Show resolved
Hide resolved
exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporterBuilder.java
Show resolved
Hide resolved
...src/testFullConfig/java/io/opentelemetry/sdk/autoconfigure/LogExporterConfigurationTest.java
Outdated
Show resolved
Hide resolved
ac8af97
to
282f2cd
Compare
dadd659
to
0899ed7
Compare
exporters/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterBuilder.java
Show resolved
Hide resolved
0899ed7
to
55c1d0e
Compare
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.
Looks good, but think we should keep the current behavior for the zipkin exporter and leave compression enabled by default.
55c1d0e
to
c46b177
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@jkwatson please take a look when you have a chance |
exporters/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterBuilder.java
Show resolved
Hide resolved
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.
Just one small javadoc nit that would be nice to add.
The
GrpcExporterBuilder
enables compression whennone
is configured.The
OkHttpExporterBuilder
has no way to disable compression vianone
once it's set (which seems wrong for a mutable builder).The
JaegerGrpcSpanExporterBuilder
andZipkinSpanExporterBuilder
have no configuration for compression at all.none
inGrpcExporterBuilder
none
inOkHttpExporterBuilder
JaegerGrpcSpanExporterBuilder
ZipkinSpanExporterBuilder
Note: All exporters except
ZipkinSpanExporterBuilder
default to compression disabled. In Zipkin the internally usedHttpSender.Builder
hascompressionEnabled = true
. This behavior was not changed, so theZipkinSpanExporterBuilder
still enables compression by default.Resolves #1652 Add GZip encoding support to Zipkin