-
Notifications
You must be signed in to change notification settings - Fork 489
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
Add kafka traces receiver #489
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Before I merge, can you update the changelog?
@joe-elliott Typically we add all the receivers to the install manifest (rendered as YAML here), but since the Kafka receiver works so differently, I don't think we can do the same thing here. WDYT?
Yeah, agree, it doesn't belong in the install manifest. Is there a place that makes sense to document this new option? |
Our docs here are really weak. IMO for now let's just have a list in the docs for tempo_instance_config, something like: # Receiver configurations are mapped directly into the OpenTelmetry receivers block.
# At least one receiver is required. Supported receivers: otlp, jaeger, kafka, [...]
# Documentation for each receiver can be found at https://github.com/open-telemetry/opentelemetry-collector/blob/7d7ae2eb34b5d387627875c498d7f43619f37ee3/receiver/README.md
receivers: |
Added the documentation and manually tested the receiver by modifying the docker-compose example. I didn't think it was worth modifying the code. Let me know if you want me to test it some other way (e.g. integration tests). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again! I'll merge once the conflicts are resolved.
355cbc0
to
c758d4a
Compare
They should be solved now |
PR Description
Adds the kafka receiver
Which issue(s) this PR fixes
Closes #484
Notes to the Reviewer
PR Checklist