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

Implement IsSampled for OpenTelemetry bridgeSpanContext #3570

Merged
merged 14 commits into from
Feb 23, 2023

Conversation

pijusn
Copy link
Contributor

@pijusn pijusn commented Jan 6, 2023

This is a PR in response to issue #3532 .

IsSampled is not exposed by OpenTracing but its implementations did (at least some; sometimes called differently; Jaeger does). Cosidering that in OpenTracing this information was accessed by casting types, this implementation is no different - instances can be casted to an interface with expected methods in order to access it. Once moved to the bridge, eventually bridge can be dropped and this information would continue being exposed through the OpenTelemetry's span context.

Resolves #3532

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 6, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: pijusn / name: Pijus Navickas (752c12a)

@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Merging #3570 (b450b22) into main (99ec432) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3570   +/-   ##
=====================================
  Coverage   81.6%   81.7%           
=====================================
  Files        166     166           
  Lines      12442   12444    +2     
=====================================
+ Hits       10162   10168    +6     
+ Misses      2065    2061    -4     
  Partials     215     215           
Impacted Files Coverage Δ
bridge/opentracing/bridge.go 63.9% <100.0%> (+0.1%) ⬆️
bridge/opentracing/internal/mock.go 67.0% <100.0%> (ø)
sdk/trace/batch_span_processor.go 81.9% <0.0%> (+1.7%) ⬆️

@pijusn pijusn force-pushed the bridge-add-is-sampled branch 2 times, most recently from ed76677 to 254f7f4 Compare January 25, 2023 07:43
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.

Please update the README with a section on how the sampled state can be accessed. Additionally, it might be worth adding an example test that accesses this method.

CHANGELOG.md Outdated Show resolved Hide resolved
@pijusn
Copy link
Contributor Author

pijusn commented Feb 1, 2023

I'm not sure example test is worthwhile here. It would contain a lot of boilerplate just to set it up and then very trivial example. Also, there is no documentation for it to be attached to. I would skip it for this component. I think example in the README should be enough (given that this is the documentation for this package).

If you do feel strongly about it, I can add it still. Let me know.

@pijusn
Copy link
Contributor Author

pijusn commented Feb 7, 2023

@MrAlias could you see the latest changes? Would be much appreciated.

@pijusn pijusn requested a review from MrAlias February 7, 2023 08:30
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
bridge/opentracing/README.md Outdated Show resolved Hide resolved
bridge/opentracing/README.md Outdated Show resolved Hide resolved
bridge/opentracing/README.md Outdated Show resolved Hide resolved
bridge/opentracing/README.md Outdated Show resolved Hide resolved
pijusn and others added 3 commits February 9, 2023 09:45
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
pijusn and others added 4 commits February 9, 2023 09:47
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@pijusn
Copy link
Contributor Author

pijusn commented Feb 9, 2023

Merged your suggestions, moved note to the unreleased, added PR ID to the note and merged the main branch into the PR. Should be up-to-date now.

@pijusn pijusn requested a review from MrAlias February 9, 2023 11:04
@pijusn
Copy link
Contributor Author

pijusn commented Feb 14, 2023

I merged main into this branch to resolve the conflict. Do you have any timeline when it could be merged (no merge rights over here, it will have to be you)? It conflicts every time something gets appended to the CHANGELOG.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 14, 2023

This still needs another review from someone else before it is ready to merge.

@MrAlias MrAlias added the pkg:bridges Related to a bridge package label Feb 17, 2023
@MrAlias MrAlias added this to the v1.14.0 milestone Feb 17, 2023
@pijusn
Copy link
Contributor Author

pijusn commented Feb 20, 2023

Thank you for reviewing the PR. I updated the branch to the latest main. It has no conflicts at the moment. I do not have the permissions required for the merge. I hope it gets merged soon 🤞🏻

@MrAlias MrAlias merged commit 69d0946 into open-telemetry:main Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:bridges Related to a bridge package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add IsSampled method to OpenTracing bridge
3 participants