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

Add jaeger support for ctxtags extraction #147

Merged
merged 1 commit into from
May 22, 2018
Merged

Add jaeger support for ctxtags extraction #147

merged 1 commit into from
May 22, 2018

Conversation

vporoshok
Copy link
Contributor

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@vporoshok
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@codecov-io
Copy link

codecov-io commented May 18, 2018

Codecov Report

Merging #147 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #147     +/-   ##
=========================================
+ Coverage   73.23%   73.33%   +0.1%     
=========================================
  Files          36       36             
  Lines        1300     1305      +5     
=========================================
+ Hits          952      957      +5     
  Misses        300      300             
  Partials       48       48
Impacted Files Coverage Δ
tracing/opentracing/id_extract.go 84.61% <100%> (+9.61%) ⬆️

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 8b8e7d5...355a479. Read the comment docs.

@domgreen
Copy link
Contributor

Would you mind adding a few tests around id_extract.go just so that nothing regresses in the future 👍

@vporoshok
Copy link
Contributor Author

Yeah, sure! Give few minutes!

@domgreen
Copy link
Contributor

Thank you 😀

} else if key == "uber-trace-id" {
parts := strings.Split(val, ":")
if len(parts) == 1 {
// for extraordinary cases
Copy link

Choose a reason for hiding this comment

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

Extraordinary cases? Maybe special cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

Under what circumstance would this happen? (I'm not very familiar with Jaeger)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not encountered such cases, so I left this comment

Copy link

Choose a reason for hiding this comment

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

If that's the case, why don't you just error or log instead of letting it through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ok if only normal case would be handled?

@domgreen
Copy link
Contributor

Did you manage to get a failing test first? I'm not convinced that there would be a failing test if we removed the code in id_extract.go.

@vporoshok
Copy link
Contributor Author

thank you for your patience, I hope that I managed to find optimal solution for testing this functionality

@domgreen domgreen merged commit e9c5d96 into grpc-ecosystem:master May 22, 2018
@vporoshok
Copy link
Contributor Author

Thank you!

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.

5 participants