Skip to content
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

kafka-go instrumentation #709

Merged
merged 19 commits into from
Apr 18, 2024
Merged

Conversation

RonFed
Copy link
Contributor

@RonFed RonFed commented Mar 10, 2024

This PR adds auto instrumentation for kafka-go library.

Instrumentation

Producer

Probing the Writer.WriteMessages function. Which produces a batch of messages, possibly on different topics.

Consumer

Probing the Reader.FetchMessage function. Which is a blocking function by its nature, we start a consumer span when it returns, and end a span in the entry probe (A typical consumer calls this function in a loop).

Producer-Consumer context propagation

Done with the headers in a kafka message, using the "traceparent" header.

General updates to support the instrumentatrion

  • Since the producer can generate a batch, the event passed from eBPF can contain multiple spans. This required extending the Probe.ProcessFn to return a slice of SpanEvents instead of a single one.
  • e2e test requires setting up kafka. Added the option to define a start.sh script in sample-job.yml
  • Improving append_item_to_slice to perform less write operations.

Example

Added example dmenonstraing the instrumentation. The example contains an http server which produces to kafka, a consumer which creates manual spans.

@RonFed RonFed marked this pull request as ready for review March 10, 2024 08:34
@RonFed RonFed requested a review from a team March 10, 2024 08:34
examples/go.mod Outdated Show resolved Hide resolved
@pellared
Copy link
Member

pellared commented Apr 2, 2024

General concerns:

  1. https://github.com/segmentio/kafka-go has no stable release. Have we heard about many users asking for this?
  2. Do we have enough velocity to maintain instrumentation for this library? Especially, that the library is not stable it may require more work than other instrumentations.

@RonFed
Copy link
Contributor Author

RonFed commented Apr 3, 2024

General concerns:

  1. https://github.com/segmentio/kafka-go has no stable release. Have we heard about many users asking for this?
  2. Do we have enough velocity to maintain instrumentation for this library? Especially, that the library is not stable it may require more work than other instrumentations.
  1. Yes, it is the second most popular kafka client from what I looked.
  2. I think so. I intend to maintain it.

@edeNFed
Copy link
Contributor

edeNFed commented Apr 10, 2024

I think this is ready to be merged. Please let me know if there are still any concerns 🙏

@damemi
Copy link
Contributor

damemi commented Apr 10, 2024

@RonFed this just has some merge conflicts (we removed the examples/go.mod and go.sum in #650)

We discussed the stability question about this in the sig meeting yesterday and how it relates to component ownership. We came down to 2 main takeaways:

  1. At this point, any new libraries that get instrumented are owned and maintained by us. We aren't a big enough project right now that we expect many users to contribute new instrumentations. If they do, we can make the case by case judgement call on if we want to own that library. For the beta milestone, we don't need a specific ownership model like collector-contrib (yet).
  2. For libraries with unstable APIs or semantic conventions, we want to put those behind a feature gate that clearly marks them as experimental. That applies to this, but also to our database/sql instrumentation. The reasoning is that users who are just expecting no-code automatic instrumentation may not be aware of unstable semantic conventions, so putting them behind a feature gate draws their attention to the semconv so they can be aware.

That said, I think we can merge this as no one seems to have a problem with (1) us owning the instrumentation and (2) it being clearly marked as under an experimental set of conventions.

For (2) I created #776

@RonFed
Copy link
Contributor Author

RonFed commented Apr 11, 2024

@damemi thanks for the feedback, those are good points.
Updated to the latest main and resolved conflicts.

@edeNFed edeNFed merged commit d6952c3 into open-telemetry:main Apr 18, 2024
17 checks passed
@MrAlias MrAlias mentioned this pull request Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants