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.

Sign up for GitHub

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

Jump to bottom

[propagator] Add Jaeger propagator #375

Merged
merged 7 commits into from
Oct 9, 2020
Merged

[propagator] Add Jaeger propagator #375

merged 7 commits into from
Oct 9, 2020

Conversation

TommyCpp
Copy link
Contributor

@TommyCpp TommyCpp commented Sep 29, 2020 •
edited by MrAlias
Loading

Some design considerations:

  • The doc mentioned that parent span id is deprecated so we will ignore this part. That's to say this part must present but we don't actually verify its content.
  • flags length can be various since we treat them as hex encoding bitmap

Closes #358

@TommyCpp TommyCpp marked this pull request as ready for review September 30, 2020 02:39
Aneurysm9
Aneurysm9 reviewed Oct 5, 2020
View reviewed changes
propagators/jaeger/doc.go Outdated Show resolved Hide resolved
propagators/jaeger/jaeger_data_test.go Outdated Show resolved Hide resolved
propagators/jaeger/jaeger_propagator.go Outdated Show resolved Hide resolved
propagators/jaeger/jaeger_propagator.go Outdated Show resolved Hide resolved
propagators/jaeger/jaeger_propagator.go Outdated Show resolved Hide resolved
@TommyCpp
[propagator] Address the comment.
480e3c4 
Change traceID so that we can verify the propagator will not just take the  low 64 bits.
MrAlias
MrAlias reviewed Oct 8, 2020
View reviewed changes
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Looks good functionally, just needs some docs. 👍

propagators/jaeger/jaeger_propagator.go Show resolved Hide resolved
propagators/jaeger/jaeger_propagator.go Show resolved Hide resolved
propagators/jaeger/jaeger_propagator.go Show resolved Hide resolved
@MrAlias MrAlias added this to the RC1 milestone Oct 9, 2020
@MrAlias MrAlias merged commit bbc03fa into open-telemetry:master Oct 9, 2020
@Aneurysm9 Aneurysm9 mentioned this pull request Oct 9, 2020
Merged
plantfansam referenced this pull request in plantfansam/opentelemetry-go-contrib Mar 18, 2022
@elsesiy @rghetia
Consistently use pointer receivers for core.Number (#375)
1ab645f 
Change all occurrences of value to pointer receivers
Add meta sys files to .gitignore
Code cleanup e.g.
- Don't capitalize error statements
- Fix ignored errors
- Fix ambiguous variable naming
- Remove unnecessary type casting
- Use named params

Fix #306
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrAlias MrAlias MrAlias approved these changes

@jmacd jmacd jmacd approved these changes

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@evantorrie evantorrie Awaiting requested review from evantorrie

@lizthegrey lizthegrey Awaiting requested review from lizthegrey

@XSAM XSAM Awaiting requested review from XSAM

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
RC1
Development

Successfully merging this pull request may close these issues.

Proposal: Add Jaeger propagator
4 participants
@TommyCpp @Aneurysm9 @jmacd @MrAlias