-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Enable support for externally-defined ID generators #1363
Conversation
* Moved the SDK's `internal.IDGenerator` interface to the `sdk/trace` package. * Added `trace.WithIDGenerator()` `TracerProviderOption`. Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
Codecov Report
@@ Coverage Diff @@
## master #1363 +/- ##
========================================
- Coverage 77.7% 77.6% -0.2%
========================================
Files 124 123 -1
Lines 6126 6131 +5
========================================
- Hits 4761 4758 -3
- Misses 1114 1122 +8
Partials 251 251
|
* Fix IDGenerator godoc comment * rename type defaultIDGenerator to randomIDGenerator * rename defIDGenerator() to defaultIDGenerator() Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
I made a note in the associated spec PR (open-telemetry/opentelemetry-specification#1006 (comment)) that I'd probably rather see two methods, where one returns a single SpanID (taking a mutex once) and one returns a pair of TraceID and SpanID (taking a mutex once). What do you think? |
Sounds useful as this seems to be how the ID generator is going to be used. Would there be a case where we only wanted to generate a trace ID? |
Yeah, I think this is indeed going to be how this will be used.
I feel like that would be the corner case anyway so throwing away that returned value wouldn't be too much of an issue. |
* NewTraceID() -> NewIDs(ctx) ** Returns both TraceID and SpanID * NewSpanID() -> NewSpanID(ctx, traceID) ** Returns only SpanID, has access to TraceID * Both methods now receive a context, from which they may extract information * startSpanInternal() updated to receive a context to pass to the ID generator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit, otherwise looks good.
Co-authored-by: Krzesimir Nowak <qdlacz@gmail.com>
internal.IDGenerator
interface to thesdk/trace
package.
trace.WithIDGenerator()
TracerProviderOption
.