contrib/segmentio/kafka.go.v0: refactor tracing code #2885
Conversation
github-actions
bot
added
the
apm:ecosystem
BenchmarksBenchmark execution time: 2024-10-15 11:17:22 Comparing candidate commit ebe1069 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 57 metrics, 2 unstable metrics. |
| require.Len(t, writtenMessages, len(messagesToWrite)) | ||
| require.Len(t, readMessages, len(messagesToWrite)) |
There was a problem hiding this comment.
Why switch from assert to require here? This is the kind of case where assert allows further assertions to run which may provide more context about what's happened that caused this to fail.
There was a problem hiding this comment.
the motivation of the require change here is because later in the test these slices are accessed by index, and if the length is not what we expect we will get a panic, making the test output noisy and confusing.
| msg := kafka.Message{ | ||
| Key: []byte("key1"), | ||
| Value: []byte("value1"), | ||
| } | ||
|
|
||
| b.ResetTimer() | ||
| for n := 0; n < b.N; n++ { | ||
| r.startSpan(nil, &msg) | ||
| tr.StartConsumeSpan(ctx, wrapMessage(&msg)) |
There was a problem hiding this comment.
So this isn't your doing; but those functions return a span that is immediately dropped on the floor... and that makes this benchmark susceptible to have the compiler optimize away a bunch of side-effects that are rendered void by this.
Usually, one wants to assign the output to a package-scoped variable so the compiler doesn't do these optimizations. It would also be good form to clean up (read: finish) those spans.
There was a problem hiding this comment.
wow thanks, good to know this :)
rarguelloF
deleted the
rarguelloF/APPSEC-53289/refactor-segmentio-kafka
branch
What does this PR do?
segmentio/kafka-go.v0: Split tracing logic into a separate package that does not import github.com/segmentio/kafka-go so it can be used in orchestrion.Additionally, it changes the docker image used for kafka tests to
confluentinc/confluent-local:7.5.0.Motivation
orchestrion cannot reuse this contrib package as it is right now because it would case an import cycle. Creating a separate package allows us to reuse this logic.
Reviewer's Checklist
Unsure? Have a question? Request a review!