Vertx 4.1 & Tracing rework #900

slinkydeveloper · 2021-05-17T10:17:11Z

Fixes #784

We're still missing eclipse-vertx/vertx-tracing#28, which is blocking us from using the new OpenTelemetry integration. We might also get rid of even more code if eclipse-vertx/vert.x#3903 gets resolved before 4.1

Proposed Changes

🎁 Vert.x 4.1
🧹 Removed our opentel integration and replaced with the brand new one provided by vert.x
🎁 Added a generic tracing test to check if tracecontext is propagated properly, both in ordered and unordered case

Release Note

Bumped vert.x to 4.1

codecov · 2021-05-24T09:40:00Z

Codecov Report

Merging #900 (f18c47c) into main (add346e) will decrease coverage by 0.24%.
The diff coverage is 69.44%.

@@             Coverage Diff              @@
##               main     #900      +/-   ##
============================================
- Coverage     76.07%   75.82%   -0.25%     
+ Complexity      441      419      -22     
============================================
  Files            78       74       -4     
  Lines          3013     2924      -89     
  Branches        133      124       -9     
============================================
- Hits           2292     2217      -75     
+ Misses          559      555       -4     
+ Partials        162      152      -10

Flag	Coverage Δ
java-unittests	`79.79% <69.44%> (-0.24%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...knative/eventing/kafka/broker/dispatcher/Main.java	`0.00% <0.00%> (ø)`
...v/knative/eventing/kafka/broker/receiver/Main.java	`0.00% <0.00%> (ø)`
...ve/eventing/kafka/broker/core/tracing/Tracing.java	`60.46% <52.17%> (+1.09%)`	⬆️
...venting/kafka/broker/core/tracing/TracingSpan.java	`90.90% <83.33%> (+18.68%)`	⬆️
...oker/receiver/CloudEventRequestToRecordMapper.java	`70.58% <83.33%> (+9.47%)`	⬆️
...ting/kafka/broker/dispatcher/RecordDispatcher.java	`93.33% <91.66%> (-0.34%)`	⬇️
...ve/eventing/kafka/broker/core/metrics/Metrics.java	`90.32% <100.00%> (+0.66%)`	⬆️
...nting/kafka/broker/core/tracing/TracingConfig.java	`86.95% <100.00%> (ø)`
...oker/dispatcher/http/HttpConsumerRecordSender.java	`60.00% <100.00%> (ø)`
...r/dispatcher/http/HttpConsumerVerticleFactory.java	`86.00% <100.00%> (+0.89%)`	⬆️
... and 2 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 add346e...f18c47c. Read the comment docs.

slinkydeveloper · 2021-05-24T11:37:55Z

I'm gonna test locally, with the new amazing dev scripts 😍 , if tracing works properly

slinkydeveloper · 2021-05-25T14:05:29Z

/hold

Tracing still doesn't work 😢

...er/src/test/java/dev/knative/eventing/kafka/broker/receiver/ReceiverTracingVerticleTest.java

slinkydeveloper · 2021-05-26T16:16:28Z

Tracing fixed!

From up to bottom, traces matches the tests TracingHeadersUsingUnorderedDeliveryWithMultipleTriggers, TracingHeadersUsingUnorderedDelivery and TracingHeadersUsingOrderedDelivery

Probably we should investigate why in the unordered case, the span is so long (sometimes it's not!), but I guess we can tackle it in a separate issue? I have the feeling this has something to do with the offset commit...

Fix #929

cc @pierDipi @devguyio @matzew

slinkydeveloper · 2021-05-26T16:18:45Z

We still need to wait for the new vert.x 4.1 release before merging this one, so i keep the hold

slinkydeveloper · 2021-05-26T16:24:53Z

Still some spans, sometimes, does not appear in zipkin in some traces, but that includes also spans from source as well... I guess that's my zipkin env? Or some race?

pierDipi

Great stuff!

/lgtm
/hold for the question, unhold if that's not possible.

.../dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/RecordDispatcher.java

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

A new simple test for tracing Updated THIRD-PARTY.txt Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper · 2021-06-03T10:02:24Z

/unhold

Rebased, this is ready for merging. @matzew @pierDipi wanna check it out?

I plan to do a followup to further improve the tracing code with some refactoring/cleanup, add b3 headers support, but I want to keep it in another PR to avoid making this one way too big.

pierDipi · 2021-06-03T19:13:21Z

...rc/main/java/dev/knative/eventing/kafka/broker/receiver/CloudEventRequestToRecordMapper.java

@@ -52,19 +50,18 @@ public CloudEventRequestToRecordMapper(final Vertx vertx) {
        }

        if (logger.isDebugEnabled()) {
-          final var span = TracingSpan.getCurrent(vertx);
+          final var span = Span.fromContextOrNull(Context.current());


What current means here? Shouldn't we pull the span from the Vertx context?

Can we use a utility to get the current span? The idea being isolating OTEL code in one module.

What current means here? Shouldn't we pull the span from the Vertx context?

Nope, I've integrated the vert.x context with the otel context apis https://github.com/eclipse-vertx/vertx-tracing/blob/master/vertx-opentelemetry/src/main/java/io/vertx/tracing/opentelemetry/VertxContextStorageProvider.java, so to retrieve spans we don't need to access the vert.x context manually.

Can we use a utility to get the current span? The idea being isolating OTEL code in one module.

TBH I think we don't need to hide the otel apis, they are straightforward and using them directly sounds reasonable to me...

pierDipi · 2021-06-03T19:14:08Z

data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/tracing/TracingSpan.java


-    final var span = getCurrent(vertx);
+  public static void decorateCurrentWithEvent(final CloudEvent event) {
+    Span span = Span.fromContextOrNull(Context.current());


pierDipi · 2021-06-04T05:57:53Z

...main/java/dev/knative/eventing/kafka/broker/dispatcher/http/HttpConsumerVerticleFactory.java

+          getOffsetManager(deliveryOrder, consumer, eventsSentCounter::increment),
+          ConsumerTracer.create(
+            ((VertxInternal) vertx).tracer(),
+            new KafkaClientOptions()
+              .setConfig(consumerConfigs)
+              .setTracingPolicy(TracingPolicy.PROPAGATE)
+          )


I don't get why we need this tracer, vertx Kafka already creates a tracer when we create a Kafka client at line 200, and it does a similar thing of https://github.com/knative-sandbox/eventing-kafka-broker/pull/900/files#diff-384d435208823cfbd29027fa5bba51ebbc6c3671ffecb12f0af13405ed9c11b1R138.
Can you add a comment on why we set tracing policy to ignore when we create a consumer and we create our own tracer?

Is it because it doesn't wait for our handler to finish before finishing the span and our handler is async?

Can you add a comment on why we set tracing policy to ignore when we create a consumer and we create our own tracer?

I was planning to clean up a bit that code in the next PR and explain why we do that and also "hide" that detail a bit, maybe behind an helper. In short, the reason is both the one you sad, which makes spans looks weird and sometimes even no-sense, and this one: #929

Just tried, there is not much I can hide, it will look weird anyway... I'll check out what can be done on vertx-kafka-client side to make this look better...

Neverthless, I've made a PR to add some comments: #964

* Update reconciler-test dependency * Increase poll.timeout from 4 to 8 minutes Updating the dependency on reconciler-test brought some changes which decreased the default timeout to 2 minutes. However, in CI sometimes the timeout needs to be longer to properly delete resources after test. Setting this explicitly to 8 minutes. * Run make generate-release

slinkydeveloper requested review from a team as code owners May 17, 2021 10:17

slinkydeveloper requested a review from fallback-notification-blocker May 17, 2021 10:17

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label May 17, 2021

knative-prow-robot added area/data-plane needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 17, 2021

slinkydeveloper mentioned this pull request May 18, 2021

[data-plane]: Bump opentelemetry-bom from 0.15.0 to 1.2.0 in /data-plane #904

Closed

slinkydeveloper requested a review from a team as a code owner May 18, 2021 12:55

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 18, 2021

slinkydeveloper force-pushed the vertx_4_1 branch from 038acc6 to d993c35 Compare May 24, 2021 09:36

slinkydeveloper changed the title ~~[WIP] Vertx 4.1~~ Vertx 4.1 May 24, 2021

knative-prow-robot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 24, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2021

slinkydeveloper force-pushed the vertx_4_1 branch from 39384ce to 0e5221b Compare May 25, 2021 15:23

matzew reviewed May 26, 2021

View reviewed changes

...er/src/test/java/dev/knative/eventing/kafka/broker/receiver/ReceiverTracingVerticleTest.java Show resolved Hide resolved

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2021

slinkydeveloper changed the title ~~Vertx 4.1~~ Vertx 4.1 & Tracing rework May 26, 2021

knative-prow-robot added the area/test label May 26, 2021

pierDipi approved these changes May 26, 2021

View reviewed changes

.../dispatcher/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/RecordDispatcher.java Outdated Show resolved Hide resolved

slinkydeveloper added 14 commits June 3, 2021 11:50

Adding opentel deps

4fd41f6

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Forgot to update opentel

04bc02a

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Some various fixes and things made for testing

fbb9bad

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Make sure metrics port doesn't get traced

8c808a5

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Fixes

b4da50b

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Cleaned some mess I did for testing

faa8018

A new simple test for tracing Updated THIRD-PARTY.txt Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Add comments to explain

e2ccf01

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Tracing

8faa384

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

if span != null

a2374f3

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

s

76859da

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

goimports

7dd73bf

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

4.1

e930c66

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Updated thirdy party

610cfd4

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Removed bad imports

f18c47c

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper force-pushed the vertx_4_1 branch from 4cd8997 to f18c47c Compare June 3, 2021 09:53

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2021

This was referenced Jun 3, 2021

[data-plane]: Bump vertx.version from 4.0.3 to 4.1.0 in /data-plane #957

Closed

[data-plane]: Bump fabric8.kubernetes.version from 5.4.0 to 5.4.1 in /data-plane #956

Merged

slinkydeveloper requested review from pierDipi and removed request for fallback-notification-blocker June 3, 2021 12:23

pierDipi reviewed Jun 3, 2021

View reviewed changes

pierDipi reviewed Jun 4, 2021

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2021

knative-prow-robot merged commit 9b66aaa into knative-extensions:main Jun 4, 2021

slinkydeveloper deleted the vertx_4_1 branch June 4, 2021 08:22

This was referenced Jun 4, 2021

Tracing does not work when using ordered delivery #929

Closed

Cleanup the Tracing class and move setup as instance method of TracingConfig #963

Merged

More comments about how tracing works for the consumer #964

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vertx 4.1 & Tracing rework #900

Vertx 4.1 & Tracing rework #900

slinkydeveloper commented May 17, 2021 •

edited

Loading

codecov bot commented May 24, 2021 •

edited

Loading

slinkydeveloper commented May 24, 2021

slinkydeveloper commented May 25, 2021

slinkydeveloper commented May 26, 2021

slinkydeveloper commented May 26, 2021

slinkydeveloper commented May 26, 2021 •

edited

Loading

pierDipi left a comment

slinkydeveloper commented Jun 3, 2021

pierDipi Jun 3, 2021

pierDipi Jun 3, 2021

slinkydeveloper Jun 4, 2021

pierDipi Jun 3, 2021

pierDipi Jun 4, 2021

pierDipi Jun 4, 2021

slinkydeveloper Jun 4, 2021 •

edited

Loading

pierDipi Jun 4, 2021

slinkydeveloper Jun 4, 2021

slinkydeveloper Jun 4, 2021

Vertx 4.1 & Tracing rework #900

Vertx 4.1 & Tracing rework #900

Conversation

slinkydeveloper commented May 17, 2021 • edited Loading

Proposed Changes

codecov bot commented May 24, 2021 • edited Loading

Codecov Report

slinkydeveloper commented May 24, 2021

slinkydeveloper commented May 25, 2021

slinkydeveloper commented May 26, 2021

slinkydeveloper commented May 26, 2021

slinkydeveloper commented May 26, 2021 • edited Loading

pierDipi left a comment

Choose a reason for hiding this comment

slinkydeveloper commented Jun 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slinkydeveloper Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slinkydeveloper commented May 17, 2021 •

edited

Loading

codecov bot commented May 24, 2021 •

edited

Loading

slinkydeveloper commented May 26, 2021 •

edited

Loading

slinkydeveloper Jun 4, 2021 •

edited

Loading