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

Support more encodings for Kafka in OTel Ingester #2580

Merged
merged 2 commits into from
Oct 24, 2020

Conversation

XSAM
Copy link
Contributor

@XSAM XSAM commented Oct 20, 2020

Resolves #2579

@XSAM XSAM requested a review from a team as a code owner October 20, 2020 07:32
@XSAM XSAM requested a review from joe-elliott October 20, 2020 07:32
@XSAM XSAM changed the title Support more encodings for kafka.consumer of OTel Ingester Support more encodings for kafka.consumer in OTel Ingester Oct 20, 2020
@XSAM XSAM force-pushed the jaeger-otel-ingester branch 2 times, most recently from 86ce768 to ce6ff12 Compare October 20, 2020 07:35
cmd/ingester/app/flags.go Outdated Show resolved Hide resolved
plugin/storage/kafka/options.go Outdated Show resolved Hide resolved
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind merging this PR (particularly if it allows @XSAM to do something interesting with the Jaeger OTel Ingester), but all of this functionality is still being discussed and this may change in the final version.

@XSAM XSAM force-pushed the jaeger-otel-ingester branch from ce6ff12 to ec31e8a Compare October 22, 2020 03:45
@mergify mergify bot dismissed joe-elliott’s stale review October 22, 2020 03:46

Pull request has been modified.

@XSAM XSAM changed the title Support more encodings for kafka.consumer in OTel Ingester Support more encodings for Kafka in OTel Ingester Oct 23, 2020
joe-elliott
joe-elliott previously approved these changes Oct 23, 2020
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I generally expect this to change, but don't think there's any issue adding this flexibility to the OTel components for now.

@XSAM looks like you need to sign some commits? Other than that LGTM

@XSAM XSAM force-pushed the jaeger-otel-ingester branch from 1f9ddec to a99043c Compare October 23, 2020 12:25
@mergify mergify bot dismissed joe-elliott’s stale review October 23, 2020 12:26

Pull request has been modified.

@codecov
Copy link

codecov bot commented Oct 23, 2020

Codecov Report

Merging #2580 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2580      +/-   ##
==========================================
- Coverage   95.31%   95.29%   -0.03%     
==========================================
  Files         208      208              
  Lines        9281     9281              
==========================================
- Hits         8846     8844       -2     
- Misses        356      358       +2     
  Partials       79       79              
Impacted Files Coverage Δ
...lugin/sampling/strategystore/adaptive/processor.go 99.07% <0.00%> (-0.93%) ⬇️

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 9f954fe...a99043c. Read the comment docs.

@joe-elliott joe-elliott self-requested a review October 23, 2020 19:07
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

@yurishkuro
Copy link
Member

Are there any documentation changes required to accompany this?

@joe-elliott
Copy link
Member

Are there any documentation changes required to accompany this?

Given that this is all likely to change and none of this is documented I think this is fine. I tried to make it clear that this is all currently up for debate and may change. I think @XSAM is just experimenting with the Otel pieces and this was something they needed.

@yurishkuro yurishkuro merged commit 20d5459 into jaegertracing:master Oct 24, 2020
@XSAM XSAM deleted the jaeger-otel-ingester branch October 25, 2020 01:20
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.

Jaeger Opentelemetry Ingester cannot correctly handle kafka.consumer.encoding flag
4 participants