Switch Jaeger remote sampler to use grpc lite #4043

pavolloffay · 2022-01-04T18:02:02Z

Resolves #4014

Notable changes:

added some methods to CodedInpotStream to support additional data types
Jaeger remote sampler support only grpc light via okhttp
switched tests from in-process grpc to armeria

anuraaga

Thanks a lot for this, looks great! Hope you could learn something about gRPC and enjoy it while writing the PR. If not, then good job anyways :)

...ler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/OkHttpGrpcService.java

anuraaga · 2022-01-05T02:23:10Z

sdk-extensions/jaeger-remote-sampler/build.gradle.kts

@@ -3,6 +3,8 @@ plugins {
  id("otel.publish-conventions")

  id("otel.animalsniffer-conventions")
+
+  id("com.squareup.wire")
 }



I don't think we need as many as the OTLP tests but can we add one test suite with the grpc dependencies used?

https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/otlp/trace/build.gradle.kts#L31

It's ok to just copy the test code instead of abstract

I have added support only for "grpc light" approach via okhttp.

Signed-off-by: Pavol Loffay <[email protected]>

codecov · 2022-01-05T13:32:35Z

Codecov Report

Merging #4043 (d5a38b5) into main (96b7895) will increase coverage by 0.01%.
The diff coverage is 81.94%.

@@             Coverage Diff              @@
##               main    #4043      +/-   ##
============================================
+ Coverage     90.12%   90.13%   +0.01%     
- Complexity     4374     4486     +112     
============================================
  Files           518      530      +12     
  Lines         13303    13788     +485     
  Branches       1276     1321      +45     
============================================
+ Hits          11989    12428     +439     
- Misses          909      926      +17     
- Partials        405      434      +29

Impacted Files	Coverage Δ
...xporter/otlp/internal/grpc/OkHttpGrpcExporter.java	`81.52% <ø> (ø)`
...er/otlp/internal/okhttp/OkHttpExporterBuilder.java	`85.41% <0.00%> (-1.63%)`	⬇️
...autoconfigure/spi/AutoConfigurationCustomizer.java	`0.00% <0.00%> (ø)`
...nfigure/AutoConfiguredOpenTelemetrySdkBuilder.java	`89.00% <33.33%> (-0.19%)`	⬇️
.../metrics/internal/descriptor/MetricDescriptor.java	`84.21% <40.00%> (-15.79%)`	⬇️
...metry/exporter/otlp/internal/CodedInputStream.java	`57.89% <50.00%> (+14.71%)`	⬆️
...er/sampler/MarshallerRemoteSamplerServiceGrpc.java	`71.42% <71.42%> (ø)`
...r/sampler/SamplingStrategyResponseUnMarshaler.java	`71.96% <71.96%> (ø)`
...ace/jaeger/sampler/JaegerRemoteSamplerBuilder.java	`91.17% <78.57%> (-8.83%)`	⬇️
...race/jaeger/sampler/DefaultGrpcServiceBuilder.java	`79.66% <79.66%> (ø)`
... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96b7895...d5a38b5. Read the comment docs.

pavolloffay · 2022-01-05T13:39:49Z

@anuraaga the coverage is low, but it seems like the "grpc lite" classes in otlp:common are not covered either.

Signed-off-by: Pavol Loffay <[email protected]>

anuraaga

Thanks @pavolloffay - I have added back code from my commit. I had to revert the change of the method signature to void. There is some more cleanup that can be done but that can happen later

pavolloffay · 2022-01-11T08:09:48Z

Do not merge yet, the tests are flaky.

pavolloffay · 2022-01-11T08:11:19Z

I am going to look at fixing them.

regarding the return type of the service, it would be appropriate propagate the result of the call to the caller.

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2022-01-11T12:44:42Z

@anuraaga I could not reproduce the failing test (unimplemented_error_server_response) locally.

while ./gradlew --no-build-cache  :sdk-extensions:jaeger-remote-sampler:cleanTest :sdk-extensions:jaeger-remote-sampler:check      ; do :; done

Now it seems that it is passing on CI.

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2022-01-11T13:23:12Z

The second to last CI job failed on the flaky test. @anuraaga any ideas why the logs are not being captured?

JaegerRemoteSamplerTest > unimplemented_error_server_response() FAILED
    org.opentest4j.AssertionFailedError: Contain the string <Server responded with UNIMPLEMENTED.> ==> None of the 7 captured log events matched the filter predicate
        at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:39)
        at app//org.junit.jupiter.api.Assertions.fail(Assertions.java:134)
        at app//io.github.netmikey.logunit.api.LogCapturer.lambda$assertContains$3(LogCapturer.java:174)
        at [email protected]/java.util.Optional.orElseGet(Optional.java:364)
        at app//io.github.netmikey.logunit.api.LogCapturer.assertContains(LogCapturer.java:173)
        at app//io.github.netmikey.logunit.api.LogCapturer.assertContains(LogCapturer.java:121)
        at app//io.opentelemetry.sdk.extension.trace.jaeger.sampler.JaegerRemoteSamplerTest.unimplemented_error_server_response(JaegerRemoteSamplerTest.java:351)

…fay/openconsensus into jaeger-remote-sampler-grpc-lite

anuraaga · 2022-01-12T03:54:36Z

@pavolloffay I added another commit 0bf83a1 to workaround issue with logunit. We've generally been able to avoid problems by using private static final Logger in classes (previous issues were when we had private final Logger). Not sure what's going on this time but worked around it anyways

…ient in time for assertion.

anuraaga · 2022-01-12T04:59:56Z

Hmm had confidence in my workaround but somehow it's not working, need to look more

jack-berg

Couple of additional comments. Looking pretty good though 👍

...s/otlp/common/src/main/java/io/opentelemetry/exporter/otlp/internal/ExporterBuilderUtil.java

jack-berg · 2022-01-14T16:50:24Z

...main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/DefaultGrpcServiceBuilder.java

+    requireNonNull(retryPolicy, "retryPolicy");
+    this.retryPolicy = retryPolicy;
+    return this;
+  }


The RetryPolicy implementation is experimental and is part of the OTLP spec. I think you should remove the methods which are not exposed in JaegerRemoteSamplerBuilder from io.opentelemetry.sdk.extension.trace.jaeger.sampler.GrpcServiceBuilder:

addRetryPolicy

addHeader

setCompression

setTimeout

Also consider removing setChannel(ManagedChannel) if you don't anticipate people using it.

I think in a soon followup we can add all except addRetryPolicy to the builder. I don't think it hurts too much keeping them.

setChannel we hope is not used but since it was public let's stick with having it for now and deprecating all of our setChannel methods in one PR

sdk-extensions/jaeger-remote-sampler/README.md

.../main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/SamplingStrategyResponse.java

...e-sampler/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/UnMarshaler.java

anuraaga · 2022-01-17T08:28:09Z

@pavolloffay FYI I added some commits, please don't lose them :)

For the record, I will be on the hook if the build is flaky after merging this.

I am hopeful for d5a38b5 my rough hypothesis is that due to HTTP/2 details, it depends on the state of the connection if there is an empty body sent and the trailer includes the status, or the headers include the status and there's no body and body read fails, which previously skipped our status handling logic. The reason I had this idea is because I saw the "could not consume server response." being logged even though none of our test cases are meant to bail on that line.

anuraaga · 2022-01-19T03:27:09Z

Thanks @pavolloffay for helping with this tedious PR!

pavolloffay · 2022-01-19T08:45:50Z

it was fun to look at the proto and grcp! Thanks for guidance as well :P

pavolloffay requested a review from anuraaga January 4, 2022 18:02

pavolloffay requested review from bogdandrutu, carlosalberto, jack-berg, jkwatson and jsuereth as code owners January 4, 2022 18:02

pavolloffay requested a review from a user January 4, 2022 18:02

pavolloffay requested a review from Oberon00 as a code owner January 4, 2022 18:02

anuraaga reviewed Jan 5, 2022

View reviewed changes

pavolloffay added 11 commits January 5, 2022 11:13

Switch Jaeger remote sampler to gRPC lite

02907aa

Signed-off-by: Pavol Loffay <[email protected]>

Abstract impl

bcf8328

Signed-off-by: Pavol Loffay <[email protected]>

Service done

4fe555f

Working example

f5ed22f

Signed-off-by: Pavol Loffay <[email protected]>

Handle gzip compression

a2d8444

Signed-off-by: Pavol Loffay <[email protected]>

Clean dependencies

746efd8

Signed-off-by: Pavol Loffay <[email protected]>

Remove dependency on proto

699f987

Signed-off-by: Pavol Loffay <[email protected]>

remove stdout

f67b7e8

Signed-off-by: Pavol Loffay <[email protected]>

Fix

f091796

Signed-off-by: Pavol Loffay <[email protected]>

Fix logger

eee0577

Signed-off-by: Pavol Loffay <[email protected]>

jcmpAPI failing

4442800

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay force-pushed the jaeger-remote-sampler-grpc-lite branch from 7aa6250 to 4442800 Compare January 5, 2022 10:31

pavolloffay added 4 commits January 5, 2022 13:42

Fix

0eec207

Signed-off-by: Pavol Loffay <[email protected]>

Fix

1874b16

Signed-off-by: Pavol Loffay <[email protected]>

Fix

05f68cd

Signed-off-by: Pavol Loffay <[email protected]>

Switch logger

7d3f7d2

Signed-off-by: Pavol Loffay <[email protected]>

Fix

f0dd031

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay mentioned this pull request Jan 5, 2022

Align Jaeger remote sampler endpoint with OTLP endpoint open-telemetry/opentelemetry-specification#2246

Merged

Support grpc netty too

a31e6c0

pavolloffay and others added 4 commits January 10, 2022 17:48

test

4c1984a

Signed-off-by: Pavol Loffay <[email protected]>

test

9ba8109

Signed-off-by: Pavol Loffay <[email protected]>

test

c11c297

Signed-off-by: Pavol Loffay <[email protected]>

Add gRPC-netty implementation

fbef2dc

anuraaga approved these changes Jan 11, 2022

View reviewed changes

pavolloffay added 2 commits January 11, 2022 13:31

some tests

0dda017

Signed-off-by: Pavol Loffay <[email protected]>

some tests

9cf4ebc

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay added 2 commits January 11, 2022 14:00

tls test

36acd9a

Signed-off-by: Pavol Loffay <[email protected]>

tls for default

d96630a

Signed-off-by: Pavol Loffay <[email protected]>

anuraaga added 2 commits January 12, 2022 03:53

Workaround logger reference issue

dfff2d7

Merge branch 'jaeger-remote-sampler-grpc-lite' of github.com:pavollof…

54185a2

…fay/openconsensus into jaeger-remote-sampler-grpc-lite

anuraaga added 3 commits January 12, 2022 04:13

Use a better logger workaround

0bf83a1

Wait for two polls since first poll may not have been processed by cl…

a79b683

…ient in time for assertion.

Don't depend on poll count for assertions.

bf9d786

shutdownNow

dd74e11

jack-berg reviewed Jan 14, 2022

View reviewed changes

anuraaga added 3 commits January 17, 2022 07:22

Cleanups

c64382d

Update logunit

2e8c202

Don't stop at body read failure

d5a38b5

jack-berg approved these changes Jan 18, 2022

View reviewed changes

anuraaga merged commit 5d521f5 into open-telemetry:main Jan 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch Jaeger remote sampler to use grpc lite #4043

Switch Jaeger remote sampler to use grpc lite #4043

pavolloffay commented Jan 4, 2022 •

edited

Loading

anuraaga left a comment

anuraaga Jan 5, 2022

pavolloffay Jan 5, 2022

codecov bot commented Jan 5, 2022 •

edited

Loading

pavolloffay commented Jan 5, 2022

anuraaga left a comment

pavolloffay commented Jan 11, 2022

pavolloffay commented Jan 11, 2022

pavolloffay commented Jan 11, 2022

pavolloffay commented Jan 11, 2022 •

edited

Loading

anuraaga commented Jan 12, 2022 •

edited

Loading

anuraaga commented Jan 12, 2022

jack-berg left a comment

jack-berg Jan 14, 2022

anuraaga Jan 17, 2022

anuraaga commented Jan 17, 2022 •

edited

Loading

anuraaga commented Jan 19, 2022

pavolloffay commented Jan 19, 2022

Switch Jaeger remote sampler to use grpc lite #4043

Switch Jaeger remote sampler to use grpc lite #4043

Conversation

pavolloffay commented Jan 4, 2022 • edited Loading

anuraaga left a comment

Choose a reason for hiding this comment

anuraaga Jan 5, 2022

Choose a reason for hiding this comment

pavolloffay Jan 5, 2022

Choose a reason for hiding this comment

codecov bot commented Jan 5, 2022 • edited Loading

Codecov Report

pavolloffay commented Jan 5, 2022

anuraaga left a comment

Choose a reason for hiding this comment

pavolloffay commented Jan 11, 2022

pavolloffay commented Jan 11, 2022

pavolloffay commented Jan 11, 2022

pavolloffay commented Jan 11, 2022 • edited Loading

anuraaga commented Jan 12, 2022 • edited Loading

anuraaga commented Jan 12, 2022

jack-berg left a comment

Choose a reason for hiding this comment

jack-berg Jan 14, 2022

Choose a reason for hiding this comment

anuraaga Jan 17, 2022

Choose a reason for hiding this comment

anuraaga commented Jan 17, 2022 • edited Loading

anuraaga commented Jan 19, 2022

pavolloffay commented Jan 19, 2022

pavolloffay commented Jan 4, 2022 •

edited

Loading

codecov bot commented Jan 5, 2022 •

edited

Loading

pavolloffay commented Jan 11, 2022 •

edited

Loading

anuraaga commented Jan 12, 2022 •

edited

Loading

anuraaga commented Jan 17, 2022 •

edited

Loading