Replace OkHttp tracing grpc backend with Vert.x #34647

geoand · 2023-07-10T13:11:53Z

Closes: #32238

TODO:

~~Remove OkHttp dependency~~
~~Fix native~~
~~Perform hardening~~ ?

quarkus-bot · 2023-07-10T13:12:19Z

/cc @brunobat (opentelemetry), @radcortez (opentelemetry)

...metry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/exporter/otlp/OtlpRecorder.java

brunobat · 2023-07-11T07:36:41Z

I'll review by the end of the week.

geoand · 2023-07-11T07:40:52Z

Thanks.

I still need to make Vert.x related improvements to this, I'll let you know when I think it's ready to go.

geoand · 2023-07-11T08:42:45Z

I think it's in a shape to be reviewed now

cescoffier

LGTM - at least the client.

.../runtime/src/main/java/io/quarkus/opentelemetry/runtime/exporter/otlp/VertxGrpcExporter.java

gastaldi · 2023-07-12T19:52:16Z

Can we remove the com.squareup.okhttp3:* dependencies from the BOM also?

geoand · 2023-07-12T19:54:03Z

No, it's used elsewhere as well (I remember an issue with Infinispan)

brunobat

Still checking the details on the PR, but one thing we must have is integration tests with the OTel Collector.
The OTel side has them for their stock exporter and we should too.

geoand · 2023-07-14T09:27:34Z

Where are those tests?

brunobat · 2023-07-14T09:42:35Z

We have a test here: https://github.com/open-telemetry/opentelemetry-java/blob/main/integration-tests/otlp/src/main/java/io/opentelemetry/integrationtest/OtlpExporterIntegrationTest.java

Also, we need to consider if we shouldn't be using a much simpler abstractions, available here:
https://github.com/open-telemetry/opentelemetry-java/tree/main/exporters/sender

This is using a new SPI that allows to plug your own client.

geoand · 2023-07-14T09:45:43Z

We have a test here: https://github.com/open-telemetry/opentelemetry-java/blob/main/integration-tests/otlp/src/main/java/io/opentelemetry/integrationtest/OtlpExporterIntegrationTest.java

Thanks

Also, we need to consider if we shouldn't be using a much simpler abstractions, available here:
https://github.com/open-telemetry/opentelemetry-java/tree/main/exporters/sender

Those are only for HTTP, they do not work for gRPC.
I am already aware of this SPI and plan to use it for #21535

brunobat · 2023-07-14T09:46:35Z

Thanks!

brunobat · 2023-07-14T09:48:29Z

I wonder if the SPI cannot be used anyway... We just need to switch based on the protocol config...
https://quarkus.io/guides/opentelemetry#quarkus-opentelemetry_quarkus.otel.exporter.otlp.traces.protocol

geoand · 2023-07-14T09:54:36Z

I wonder if the SPI cannot be used anyway

I don't see how as the gRPC exporters don't use HttpSender at all

geoand · 2023-07-14T10:56:08Z

Seems my IntelliJ setup is completely busted, so I won't be adding the test today. If you want to take a swing at it, feel free to push a new commit into this PR

brunobat · 2023-07-17T08:48:34Z

looking at the test right now

geoand · 2023-07-17T08:59:30Z

I was actually able to fix my IntelliJ setup so I am looking into adding the required test

geoand · 2023-07-17T09:34:00Z

As part of the test work, I'll also probably add TSL config support - looking into both now

brunobat · 2023-07-17T09:53:48Z

Ok @geoand. Will work on the baggage problem instead.

geoand · 2023-07-17T09:54:24Z

Sounds good!

geoand · 2023-07-17T12:49:34Z

PR updated with an integration test uses the same testing philosophy as the original opentelemetry-java but has two key differences:

Uses Vert.x instead of Armeria
It's an actual integration test in the sense that it exercises a Quarkus application that uses the exporter (meaning that we also test Quarkus configuration and all)

Furthermore, the final commit also adds support for TLS configuration of the exporter.

geoand · 2023-07-17T12:53:30Z

...metry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/exporter/otlp/OtlpRecorder.java

+                                || !options.getPemKeyCertOptions().getCertPaths().isEmpty()
+                                || !options.getPemKeyCertOptions().getKeyPaths().isEmpty()) {
+                            options.setSsl(true);
+                            options.setUseAlpn(true);


@cescoffier I found that is is necessary, can you confirm?

I mean that based on the error message I was seeing, it seemed like Alpn is necessary when using HTTP2. Did I understand correctly?

brunobat

Excellent. Getting rid of Armeria was perfect.

.../java/io/quarkus/opentelemetry/runtime/config/runtime/exporter/OtlpExporterTracesConfig.java

Also add TLS support to the exporter

.../java/io/quarkus/opentelemetry/runtime/config/runtime/exporter/OtlpExporterTracesConfig.java

quarkus-bot · 2023-07-17T17:34:16Z

Failing Jobs - Building `bd6cfd6`

Status	Name	Step	Failures	Logs	Raw logs
✔️	Maven Tests - JDK 11
⌛	Maven Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs

geoand · 2023-07-17T17:35:48Z

@brunobat all good to merge?

brunobat

Thanks @geoand !

geoand · 2023-07-18T08:24:21Z

🙏🏼

quarkus-bot bot added the area/tracing label Jul 10, 2023

geoand mentioned this pull request Jul 10, 2023

OOM in Quarkus 3.0.0.Beta1 caused by okio via OpenTelemetry #32238

Closed

geoand added the release/noteworthy-feature label Jul 10, 2023

radcortez reviewed Jul 10, 2023

View reviewed changes

...metry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/exporter/otlp/OtlpRecorder.java Outdated Show resolved Hide resolved

geoand force-pushed the #32238 branch from 76f2094 to bf4a4cc Compare July 11, 2023 08:42

geoand requested review from radcortez, brunobat and cescoffier July 11, 2023 08:42

geoand mentioned this pull request Jul 11, 2023

Properly construct instance of LateBoundBatchSpanProcessor #34545

Merged

cescoffier reviewed Jul 11, 2023

View reviewed changes

.../runtime/src/main/java/io/quarkus/opentelemetry/runtime/exporter/otlp/VertxGrpcExporter.java Show resolved Hide resolved

geoand mentioned this pull request Jul 12, 2023

Provide OpenTelemetry OTLP/HTTP exporter #21535

Closed

radcortez approved these changes Jul 12, 2023

View reviewed changes

geoand marked this pull request as ready for review July 12, 2023 11:05

geoand assigned brunobat Jul 13, 2023

cescoffier approved these changes Jul 13, 2023

View reviewed changes

brunobat requested changes Jul 14, 2023

View reviewed changes

geoand unassigned brunobat Jul 14, 2023

Take TracingPolicy.IGNORE into account

7fa8659

geoand force-pushed the #32238 branch from bf4a4cc to 8889a88 Compare July 17, 2023 12:47

quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Jul 17, 2023

geoand requested a review from brunobat July 17, 2023 12:49

geoand commented Jul 17, 2023

View reviewed changes

brunobat reviewed Jul 17, 2023

View reviewed changes

.../java/io/quarkus/opentelemetry/runtime/config/runtime/exporter/OtlpExporterTracesConfig.java Show resolved Hide resolved

geoand added 3 commits July 17, 2023 16:55

Replace OkHttp tracing backend with Vert.x

c3e43e9

Remove OkHttp dependency from OpenTelemetry extension

1602ee1

Add integration test for Vert.x gRPC exporter

bd6cfd6

Also add TLS support to the exporter

geoand force-pushed the #32238 branch from 8889a88 to bd6cfd6 Compare July 17, 2023 13:59

gastaldi reviewed Jul 17, 2023

View reviewed changes

.../java/io/quarkus/opentelemetry/runtime/config/runtime/exporter/OtlpExporterTracesConfig.java Show resolved Hide resolved

brunobat approved these changes Jul 18, 2023

View reviewed changes

geoand merged commit abbbe5f into quarkusio:main Jul 18, 2023

geoand deleted the #32238 branch July 18, 2023 08:24

quarkus-bot bot added the kind/bugfix label Jul 18, 2023

quarkus-bot bot added this to the 3.3 - main milestone Jul 18, 2023

snazy mentioned this pull request Jul 18, 2023

Remove OTel/Quarkus/OkHttp OOM workaround w/ Quarkus 3.4 projectnessie/nessie#7256

Closed

geoand changed the title ~~Replace OkHttp tracing backend with Vert.x~~ Replace OkHttp tracing grpc backend with Vert.x Jul 19, 2023

radcortez mentioned this pull request Jul 31, 2023

Quarkus opentelemtry extension still depends on an old version of OkHttp #35121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace OkHttp tracing grpc backend with Vert.x #34647

Replace OkHttp tracing grpc backend with Vert.x #34647

geoand commented Jul 10, 2023 •

edited

Loading

quarkus-bot bot commented Jul 10, 2023

brunobat commented Jul 11, 2023

geoand commented Jul 11, 2023

geoand commented Jul 11, 2023

cescoffier left a comment

gastaldi commented Jul 12, 2023

geoand commented Jul 12, 2023

brunobat left a comment

geoand commented Jul 14, 2023

brunobat commented Jul 14, 2023

geoand commented Jul 14, 2023

brunobat commented Jul 14, 2023

brunobat commented Jul 14, 2023

geoand commented Jul 14, 2023

geoand commented Jul 14, 2023

brunobat commented Jul 17, 2023

geoand commented Jul 17, 2023 •

edited

Loading

geoand commented Jul 17, 2023

brunobat commented Jul 17, 2023

geoand commented Jul 17, 2023

geoand commented Jul 17, 2023 •

edited

Loading

geoand Jul 17, 2023

geoand Jul 17, 2023

brunobat left a comment

quarkus-bot bot commented Jul 17, 2023

geoand commented Jul 17, 2023

brunobat left a comment

geoand commented Jul 18, 2023

Replace OkHttp tracing grpc backend with Vert.x #34647

Replace OkHttp tracing grpc backend with Vert.x #34647

Conversation

geoand commented Jul 10, 2023 • edited Loading

quarkus-bot bot commented Jul 10, 2023

brunobat commented Jul 11, 2023

geoand commented Jul 11, 2023

geoand commented Jul 11, 2023

cescoffier left a comment

Choose a reason for hiding this comment

gastaldi commented Jul 12, 2023

geoand commented Jul 12, 2023

brunobat left a comment

Choose a reason for hiding this comment

geoand commented Jul 14, 2023

brunobat commented Jul 14, 2023

geoand commented Jul 14, 2023

brunobat commented Jul 14, 2023

brunobat commented Jul 14, 2023

geoand commented Jul 14, 2023

geoand commented Jul 14, 2023

brunobat commented Jul 17, 2023

geoand commented Jul 17, 2023 • edited Loading

geoand commented Jul 17, 2023

brunobat commented Jul 17, 2023

geoand commented Jul 17, 2023

geoand commented Jul 17, 2023 • edited Loading

geoand Jul 17, 2023

Choose a reason for hiding this comment

geoand Jul 17, 2023

Choose a reason for hiding this comment

brunobat left a comment

Choose a reason for hiding this comment

quarkus-bot bot commented Jul 17, 2023

Failing Jobs - Building bd6cfd6

geoand commented Jul 17, 2023

brunobat left a comment

Choose a reason for hiding this comment

geoand commented Jul 18, 2023

geoand commented Jul 10, 2023 •

edited

Loading

geoand commented Jul 17, 2023 •

edited

Loading

geoand commented Jul 17, 2023 •

edited

Loading

Failing Jobs - Building `bd6cfd6`