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.

Sign up for GitHub

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

Jump to bottom

Require the SDK to provide custom ID generation #1006

Merged
merged 2 commits into from
Dec 16, 2020
Merged

Require the SDK to provide custom ID generation #1006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43652ff
Require the SDK to provide custom ID generation
NathanielRN Sep 24, 2020
4fbb664
Merge branch 'master' into custom-id-generation
yurishkuro Dec 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ Updates:
- Resource's service.name MUST have a default value, service.instance.id is not
required.
([#1269](https://github.com/open-telemetry/opentelemetry-specification/pull/1269))
- Add requirement that the SDK allow custom generation of Trace IDs and Span IDs
([#1006](https://github.com/open-telemetry/opentelemetry-specification/pull/1006))

## v0.7.0 (11-18-2020)

Expand Down
1 change: 1 addition & 0 deletions spec-compliance-matrix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ status of the feature is not known.
|Allow samplers to modify tracestate | | + | | + | | + | | + | | | + |
|ShouldSample gets full parent Context | | + | + | + | | + | | | | | + |
|[New Span ID created also for non-recording Spans](specification/trace/sdk.md#sdk-span-creation) | | | | + | | | | | | | + |
|SDK Trace & Span ID generation is customizable| + | + | + | + | | | | | | + | |

## Baggage

Expand Down
17 changes: 17 additions & 0 deletions specification/trace/sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,23 @@ Note: Implementation-wise, this could mean that `Tracer` instances have a
reference to their `TracerProvider` and access configuration only via this
reference.

The SDK MUST by default randomly generate the bytes for both the `TraceId` and
the `SpanId`.

The SDK MUST provide a mechanism for customizing the way IDs are generated for
Copy link
Member

@Oberon00 Oberon00 Sep 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

"provide a mechanism" is a bit too vague IMHO. We should maybe say that some generator functions must be set separately for trace and span id, and whether they should receive any input arguments (such as the same input as the sampler minus trace ID for trace ID generation, the same input as the sampler plus decision for span ID generation).

Maybe ID generation (especially trace ID generation) should also be integrated with the sampler, this could be a nice performance gain. See #864.

Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, here's the approach we're taking with .NET

https://github.com/open-telemetry/opentelemetry-dotnet/pull/1268/files#diff-af68eed4c4ee2630b12fc2eb6feb5c27R37

It listens for new Spans (Activities) and overrides the ID for local roots. This is basically the best we can do without a change to the .NET runtime itself.

Not sure how to phrase it to handle both the case where we define an interface of generator functions, or for this sort of runtime callback. The important part is the IDs can be customized somehow.

Copy link
Member

Choose a reason for hiding this comment

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

Changing an already set ID seems like a waste, as the generation is not free. Also, what if other components already got that ID and assume the span will keep it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The callback happens right when the activity is created so it (as far as we understand) is safe as far as visibility goes. It better be ;)

It is wasteful to regenerate the ID, but I guess this is what .NET wants to expose. If we required a customization point before that, it's a .NET runtime change, not an OTel change. As much as I'd like that option, I'm not optimistic it's possible to drive through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also agree that it's hard to commit all the SDKs to following a similar pattern if we don't know how difficult it will be to get all the languages compliant. For Python it was really easy, for .NET it sounds like it wasn't...

All we know is that we do want this functionality, so that although vague, it encourages the SDKs to push towards a good thing? Regardless, any suggestions on the wording that gives us the best of both worlds is very welcome as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it may be hard to specify this, as we don't have this implemented (or prototyped) in a few languages. We can work on trying to be more formal about the requirements as an After-GA goal.

both the `TraceId` and the `SpanId`.
NathanielRN marked this conversation as resolved.
Show resolved Hide resolved

The SDK MAY provide this functionality by allowing custom implementations of
an interface like `IdsGenerator` below, which provides extension points for two
methods, one to generate a `SpanID` and one to generate a `TraceId`.

```
IdsGenerator {
String generateSpanId()
String generateTraceId()
}
```

### Shutdown

This method provides a way for provider to do any cleanup required.
Expand Down