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

Added methods for SpanID and TraceID on bridgeSpanContext #3966

Merged
merged 14 commits into from Apr 25, 2023

Conversation

Kaushal28
Copy link
Contributor

Resolves: #3954

Added methods for SpanID and TraceID on bridgeSpanContext and corresponding unit test case.

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

  1. It is not documented how to use it. See bridge/opentracing/README.md and the "extending" section.
  2. It would be good to add some tests or at least comments in code that would make sure that someone would not remove these methods in future.
  3. Missing changelog entry.

EDIT: See #3570 for reference.

@Kaushal28
Copy link
Contributor Author

Updated the PR based on your suggestions. However, I am not exactly sure about the usage of the methods. So I have added a similar description as the reference you provided. So suggestions/feedback would be really helpful there. Also, updated the test case based on that, added comments on the methods and added entry in CHANGELOG.md.

bridge/opentracing/bridge_test.go Outdated Show resolved Hide resolved
bridge/opentracing/bridge_test.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #3966 (4038159) into main (6acade8) will decrease coverage by 0.1%.
The diff coverage is 66.6%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3966     +/-   ##
=======================================
- Coverage   82.2%   82.2%   -0.1%     
=======================================
  Files        175     175             
  Lines      13067   13065      -2     
=======================================
- Hits       10744   10742      -2     
  Misses      2102    2102             
  Partials     221     221             
Impacted Files Coverage Δ
bridge/opentracing/bridge.go 63.7% <66.6%> (-0.2%) ⬇️

@MadVikingGod
Copy link
Contributor

I'm wondering, instead of chasing down all the methods that SpanContext exposes, should we just have the bridgeSpanContext be able to produce the otel SpanContext?

We can't drop IsSampled, but this might head off the need for IsValid, IsRemote, TraceFlags, TraceState, and anything else that might be added in the future.

@Kaushal28
Copy link
Contributor Author

@MadVikingGod, @pellared, I made changes as you suggested. I added otel.SpanContext to bridgeSpanContext struct as anonymous field and promoted all the available methods on otel.SpanContext to bridgeSpanContext so that any method can be accessed similar to how isSampled() is currently being accessed. Let me know this change makes sense. Any suggestions/feedback would be really helpful. Thanks.

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
@pellared
Copy link
Member

pellared commented Apr 5, 2023

bridge/opentracing/README.md:64:3 MD047/single-trailing-newline Files should end with a single newline character

CHANGELOG.md Outdated Show resolved Hide resolved
bridge/opentracing/README.md Outdated Show resolved Hide resolved
Co-authored-by: Robert Pająk <pellared@hotmail.com>
bridge/opentracing/bridge_test.go Outdated Show resolved Hide resolved
bridge/opentracing/bridge_test.go Outdated Show resolved Hide resolved
@Kaushal28
Copy link
Contributor Author

@pellared @MrAlias I have fixed the test case as suggested. Please have a look. Thanks.

@Kaushal28 Kaushal28 requested a review from MrAlias April 14, 2023 16:09
@Kaushal28
Copy link
Contributor Author

Hi @MrAlias , I have made the required changes, could you please take a look and provide any feedback?

@MrAlias
Copy link
Contributor

MrAlias commented Apr 25, 2023

@Kaushal28 please update this branch with main so it can be merged.

@Kaushal28
Copy link
Contributor Author

@MrAlias I updated this branch but a CI step failed while uploading codecov report with error Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.. Not sure whether it's temporary error. How can I re-run these checks?

@MrAlias MrAlias merged commit 86f3258 into open-telemetry:main Apr 25, 2023
20 of 21 checks passed
@MrAlias MrAlias mentioned this pull request Apr 27, 2023
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.

[OpenTracing Bridge] Add methods to expose TraceID and SpanID from bridgeSpanContext
4 participants