This repository has been archived by the owner on Jan 26, 2024. It is now read-only.
ScopeManager with default thread_local implementation #124
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Includes only the suggested ScopeManager interface and related classes. No implementation is provided but headers were included in a compilation unit for testing. Clang-format was run
All interfaces have been implemented with stub implementations, but do compile. Tests have been setup but not written for ScopeManager.
The previous idea was to have ScopeManager as a Utility like class with the thread_local implementation. After reviewing the Java implementation, I have made the ScopeManager an interface and a seperate ThreadLocalScopeManager class will provide the thread_local implementation. The reference based interface has been replaced with shared_ptr. This is to allow the Span to be consumed from the ScopeManager and have its lifetime managed by the application/library code as appropriate. E.g. consider an application that keeps the Span alive until an Async signal is received.
- Change the Scope to simply take a callback and not require friendship with ScopeManager. - ScopeManager can now define implementation when instantiating Scope. - Updated tests/stubs to reflect new Scope interface. - Minor documentation fixes
- Created a ThreadLocalScopeManager which implements ScopeManager - Headers with declarations - Empty stubs and tests - Updated ScopeManager docs - Updated build files for new component
- Tracer to instantiate its own ScopeManager (for backwards compatibility) - Implementation provided but can be overriden by Tracer implementations - Fixed minor errors in original tests
eyjohn
changed the title
- GCC complained about function name/type of ScopeManager - Typo in docs - On ubuntu, tests required pthreads
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR for #97 to provide span propagation using the ScopeManager approach.
I've based this on the Java version https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/ScopeManager.java including making it part of the Tracer (originally I had planned to keep it a stand-alone utility).
Most of the design decisions are in-line with the Java interface, the following are probably worth highlighting:
1. ScopeManagers use shared_ptr rather than unique_ptr/reference
The rationale here is that the nature of sharing a Span over a scope, means that multiple downstream consumers may access it, OR even take ownership of it. Therefore a conversion to shared_ptr is required before a ScopeManager is used
2. Integration with Tracer
In-line with the Java design, the ScopeManager is accessible via the
Tracer::ScopeManager()method, which can be overriden by tracers.For backwards compatibility, I've had to add some logic to the Tracer interface and unfortunately it is no longer a pure interface.
3. Scope
I've added a "guard-like" Scope object to track the
ScopeManager::Activatescope but kept the constraints quite open (very difficult to enforce at interface level), e.g. avoiding moving or storing it outside of the stack.The specific implementations of ScopeManagers may have different constraints so I've left it to the ScopeManager documentation to provide the exact behaviour (such as for the thread local one, all construction/destruction must be in the same thread).
I hope you don't mind that I've delivered a complete PR prior to major discussion, I was hoping given that this is very similar to the Java one that there would be less design concerns.
On a final note, can you please help with what changes are needed to pass the PR validation?
Thanks