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

Enable support for externally-defined ID generators #1363

Merged
merged 6 commits into from
Dec 10, 2020

Conversation

Aneurysm9
Copy link
Member

  • Moved the SDK's internal.IDGenerator interface to the sdk/trace
    package.
  • Added trace.WithIDGenerator() TracerProviderOption.

* 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
Copy link

codecov bot commented Nov 21, 2020

Codecov Report

Merging #1363 (13a268d) into master (0021ab0) will decrease coverage by 0.1%.
The diff coverage is 86.6%.

Impacted file tree graph

@@           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             
Impacted Files Coverage Δ
sdk/trace/provider.go 92.3% <33.3%> (-2.1%) ⬇️
sdk/trace/id_generator.go 100.0% <100.0%> (ø)
sdk/trace/span.go 94.9% <100.0%> (+<0.1%) ⬆️
sdk/trace/tracer.go 100.0% <100.0%> (ø)
sdk/trace/batch_span_processor.go 74.5% <0.0%> (-5.9%) ⬇️

sdk/trace/id_generator.go Outdated Show resolved Hide resolved
sdk/trace/id_generator.go Outdated Show resolved Hide resolved
* Fix IDGenerator godoc comment
* rename type defaultIDGenerator to randomIDGenerator
* rename defIDGenerator() to defaultIDGenerator()

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
@jmacd
Copy link
Contributor

jmacd commented Nov 24, 2020

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?

@krnowak
Copy link
Member

krnowak commented Nov 25, 2020

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?

@MrAlias
Copy link
Contributor

MrAlias commented Dec 3, 2020

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?

Yeah, I think this is indeed going to be how this will be used.

Would there be a case where we only wanted to generate a trace ID?

I feel like that would be the corner case anyway so throwing away that returned value wouldn't be too much of an issue.

sdk/trace/id_generator.go Outdated Show resolved Hide resolved
* 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
@MrAlias MrAlias requested review from krnowak and XSAM December 8, 2020 19:16
Copy link
Member

@krnowak krnowak left a 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.

sdk/trace/id_generator.go Outdated Show resolved Hide resolved
Co-authored-by: Krzesimir Nowak <qdlacz@gmail.com>
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.

Provide extensibility of the SDK to support vendor specific ID Generators
7 participants