Require the SDK to provide custom ID generation #1006

NathanielRN · 2020-09-25T03:19:25Z

Fixes #896

Changes

Make the SDKs commit to providing a way for IDs to be customizable by including it in the specifications.

How they do so is up to the SDKs individually.

Providing this will solve the problem of having traces incompatible with some backends as discussed in the linked issue.

specification/trace/sdk.md

Actually, on second thaught, we should be more specific here.

Oberon00 · 2020-09-25T08:41:49Z

specification/trace/sdk.md

@@ -196,6 +196,12 @@ 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 provide a mechanism for customizing the way IDs are generated for


"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.

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.

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?

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.

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!

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.

andrewhsu · 2020-09-25T16:25:06Z

from the issue triage mtg today, labelling this as after-ga since the corresponding issue is also marked as after-ga.

NathanielRN · 2020-09-25T20:46:38Z

@andrewhsu @Oberon00 Just curious, does that mean we can't merge in this change until a later point? This is already a reality for the Java & JavaScript OTel SDKs, and is being worked on for the Python & .NET SDks.

The goal of getting this description in now rather than later would be to make for an easier argument when proposing these changes for the rest of the SDKs. (Although it could be worked on after GA too).

tigrannajaryan · 2020-09-25T20:53:18Z

@NathanielRN issues marked release:after-ga can be merged to the spec after the release since they are considered to be non-breaking. However they are no longer being worked on from the moment spec freeze is announced and to the moment GA is released. We want to establish a non-moving target for language maintainers. Individuals are of course free to continue working on such issues and submit corresponding PRs after the GA.

anuraaga · 2020-09-26T04:44:52Z

@tigrannajaryan Can you clarify this?

However they are no longer being worked on from the moment spec freeze is announced and to the moment GA is released.

You say they can be merged but they are no longer being worked on, which would imply not being merged? Want to make sure I understand :)

For this issue, since it affects compatibility with backends, I think if this is not merged for GA, at least there has to be a check to make sure all languages can add the customizability without breaking changes. If that's not possible, then that language would be incompatible with the backend until a major version upgrade which might take years. I guess that could be against the goals of OTel.

anuraaga · 2020-09-29T14:14:45Z

Just want to point out that there is some difficulty in getting X-Ray compatibility in language SDKs because we haven't gotten this spec'd yet - if it was there, we reference the spec and everyone spends a lot less time / cognitive load and can be more productive. Is there some way we could prioritize this? 🙏

open-telemetry/opentelemetry-python#1153 (comment)

tigrannajaryan · 2020-09-29T15:15:57Z

You say they can be merged but they are no longer being worked on, which would imply not being merged? Want to make sure I understand :)

I wrote that they can be merged after the release. So they cannot be merged from the moment when freeze is announced and until the release is made.

anuraaga · 2020-09-29T15:32:26Z

Thanks I misread that for some reason.

A bit worried on this one since I suspect the issue / PR was labeled after-ga mostly automatically since I don't see much evidence of checking to confirm this won't be a breaking change in any of the languages. TBH I didn't prioritize sending a PR mainly because I didn't know when the real freeze date is, which I guess was last Friday. It definitely doesn't seem to have been as well publicized as #792 with the pinned issue. It's ok but wondering if there's anything we can do here to be confident that OTel can support X-Ray in all the languages. @bogdandrutu you happen to be assigned here but feel like championing this or any workaround?

github-actions · 2020-10-07T03:24:36Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

anuraaga

I think this is an important point with a chance of breaking compatibility with certain backends in the long term if not addressed. It's a fairly lightweight change, ensuring there is a mechanism but not overdefining it, so seems safe. Going to leave my check and hope others agree :D

arminru · 2020-10-20T12:43:10Z

@NathanielRN Please add an entry in CHANGELOG.md and spec-compliance-matrix.md and update the PR title to match the content.

NathanielRN · 2020-10-20T14:38:45Z

@arminru Done! ✅

arminru

Thanks! Please add an entry here as well for tracking implementations adopting that requirement:
https://github.com/open-telemetry/opentelemetry-specification/blob/master/spec-compliance-matrix.md#traces
I think the Traces::Span section would be appropriate.

arminru · 2020-10-20T14:43:21Z

CHANGELOG.md

@@ -55,6 +55,8 @@ New:
  ([#1074](https://github.com/open-telemetry/opentelemetry-specification/pull/1074))
 - Add semantic conventions for system metrics
  ([#937](https://github.com/open-telemetry/opentelemetry-specification/pull/937))
+- Add requirement that the SDK provide custom generation of Trace IDs and Span IDs


Suggested change

- Add requirement that the SDK provide custom generation of Trace IDs and Span IDs

- Add requirement that the SDK provides custom generation of Trace IDs and Span IDs

Or actually "allows for customizing", since the SDK doesn't do so itself.

"Allows" sounds like a good idea! I think it's correct to not include the -s just based on gut feeling. In a google search I found this on the subject but I might be wrong and I can change it if needed!

anuraaga

Just a small point, thanks!

anuraaga · 2020-11-21T04:18:34Z

specification/trace/sdk.md

+The SDK SHOULD by default randomly generate the bytes for both the `TraceId` and
+the `SpanId`.
+
+The SDK SHOULD provide this functionality by allowing custom implementations of


I would use MAY - the interface is an example to help language authors understand the concept, but I don't think we have to recommend a particular interface which may not be idiomatic in a particular language.

Sounds good, changed!

anuraaga · 2020-11-21T04:50:15Z

specification/trace/sdk.md

+The SDK MUST provide a mechanism for customizing the way IDs are generated for
+both the `TraceId` and the `SpanId`.
+
+The SDK SHOULD by default randomly generate the bytes for both the `TraceId` and


Ah one small point, can you move this to be above the paragraph about customization? Right now we have this non-customization point sandwiched between the two.

And I guess this is a MUST not SHOULD (though not sure)

Moved it up!

Also changed it to MUST :)

github-actions · 2020-11-29T03:23:44Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

github-actions · 2020-12-07T03:27:39Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

anuraaga · 2020-12-07T03:40:42Z

@NathanielRN Can you rebase? Thanks!

@jmacd Looks like there are a lot of approvals, hopefully this can get merged after the rebase

NathanielRN · 2020-12-07T04:46:14Z

Rebase is done 👍

github-actions · 2020-12-15T03:29:56Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

anuraaga · 2020-12-15T04:33:35Z

@jmacd @open-telemetry/specs-trace-approvers This is becoming a weekly rebase now - can we merge it?

NathanielRN · 2020-12-15T20:37:17Z

@jmacd @open-telemetry/specs-trace-approvers one of the concerns of merging this was brought up by @tigrannajaryan regarding this moving the target for languages trying to be spec-compliant for GA, I went through some of the languages and updated the compliance matrix for this PR to note that JavaScript and Go have both added this functionality.

Hopefully, that can motivate merging this in sooner because languages have been adopting this change quickly!

NathanielRN requested review from a team as code owners September 25, 2020 03:19

github-actions bot assigned bogdandrutu Sep 25, 2020

Oberon00 reviewed Sep 25, 2020

View reviewed changes

specification/trace/sdk.md Outdated Show resolved Hide resolved

Oberon00 previously approved these changes Sep 25, 2020

View reviewed changes

Oberon00 reviewed Sep 25, 2020

View reviewed changes

andrewhsu added the release:after-ga Not required before GA release, and not going to work on before GA label Sep 25, 2020

github-actions bot added the Stale label Oct 7, 2020

anuraaga approved these changes Oct 14, 2020

View reviewed changes

bogdandrutu approved these changes Oct 14, 2020

View reviewed changes

bogdandrutu removed the Stale label Oct 14, 2020

NathanielRN force-pushed the custom-id-generation branch from 7ac0da9 to 87c7c45 Compare October 14, 2020 16:48

carlosalberto added area:sdk Related to the SDK spec:trace Related to the specification/trace directory labels Oct 15, 2020

NathanielRN force-pushed the custom-id-generation branch from 87c7c45 to 704efba Compare October 20, 2020 14:36

NathanielRN changed the title ~~SDKs should support customizable IDs for backends~~ Require the SDK to provide custom ID generation Oct 20, 2020

NathanielRN force-pushed the custom-id-generation branch from 704efba to 4e9c012 Compare October 20, 2020 14:38

arminru reviewed Oct 20, 2020

View reviewed changes

jmacd approved these changes Nov 20, 2020

View reviewed changes

anuraaga approved these changes Nov 21, 2020

View reviewed changes

NathanielRN force-pushed the custom-id-generation branch 3 times, most recently from f6050f1 to ccd5f52 Compare November 21, 2020 04:24

anuraaga reviewed Nov 21, 2020

View reviewed changes

jmacd mentioned this pull request Nov 24, 2020

Enable support for externally-defined ID generators open-telemetry/opentelemetry-go#1363

Merged

github-actions bot closed this Nov 29, 2020

anuraaga reopened this Nov 29, 2020

NathanielRN force-pushed the custom-id-generation branch from a94e15d to c946d3c Compare November 29, 2020 05:12

github-actions bot closed this Dec 7, 2020

anuraaga reopened this Dec 7, 2020

NathanielRN force-pushed the custom-id-generation branch from c946d3c to a8a9253 Compare December 7, 2020 04:45

github-actions bot removed the Stale label Dec 8, 2020

github-actions bot added the Stale label Dec 15, 2020

NathanielRN force-pushed the custom-id-generation branch 2 times, most recently from 7b60caa to 2581331 Compare December 15, 2020 04:29

Require the SDK to provide custom ID generation

43652ff

NathanielRN force-pushed the custom-id-generation branch from 2581331 to 43652ff Compare December 15, 2020 20:32

Merge branch 'master' into custom-id-generation

4fbb664

yurishkuro merged commit 418016d into open-telemetry:master Dec 16, 2020

NathanielRN deleted the custom-id-generation branch December 17, 2020 17:11

NathanielRN mentioned this pull request Jul 8, 2021

REQUEST: New membership for NathanielRN open-telemetry/community#777

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require the SDK to provide custom ID generation #1006

Require the SDK to provide custom ID generation #1006

NathanielRN commented Sep 25, 2020

Oberon00 Sep 25, 2020 •

edited

Loading

anuraaga Sep 25, 2020

Oberon00 Sep 25, 2020

anuraaga Sep 25, 2020

NathanielRN Sep 25, 2020

carlosalberto Nov 11, 2020

andrewhsu commented Sep 25, 2020

NathanielRN commented Sep 25, 2020 •

edited

Loading

tigrannajaryan commented Sep 25, 2020 •

edited

Loading

anuraaga commented Sep 26, 2020

anuraaga commented Sep 29, 2020

tigrannajaryan commented Sep 29, 2020

anuraaga commented Sep 29, 2020

github-actions bot commented Oct 7, 2020

anuraaga left a comment

arminru commented Oct 20, 2020

NathanielRN commented Oct 20, 2020

arminru left a comment

arminru Oct 20, 2020 •

edited

Loading

NathanielRN Oct 20, 2020

anuraaga left a comment

anuraaga Nov 21, 2020

NathanielRN Nov 21, 2020

anuraaga Nov 21, 2020

NathanielRN Nov 21, 2020

github-actions bot commented Nov 29, 2020

github-actions bot commented Dec 7, 2020

anuraaga commented Dec 7, 2020

NathanielRN commented Dec 7, 2020

github-actions bot commented Dec 15, 2020

anuraaga commented Dec 15, 2020

NathanielRN commented Dec 15, 2020

	- Add requirement that the SDK provide custom generation of Trace IDs and Span IDs
	- Add requirement that the SDK provides custom generation of Trace IDs and Span IDs

Require the SDK to provide custom ID generation #1006

Require the SDK to provide custom ID generation #1006

Conversation

NathanielRN commented Sep 25, 2020

Changes

Oberon00 Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewhsu commented Sep 25, 2020

NathanielRN commented Sep 25, 2020 • edited Loading

tigrannajaryan commented Sep 25, 2020 • edited Loading

anuraaga commented Sep 26, 2020

anuraaga commented Sep 29, 2020

tigrannajaryan commented Sep 29, 2020

anuraaga commented Sep 29, 2020

github-actions bot commented Oct 7, 2020

anuraaga left a comment

Choose a reason for hiding this comment

arminru commented Oct 20, 2020

NathanielRN commented Oct 20, 2020

arminru left a comment

Choose a reason for hiding this comment

arminru Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 29, 2020

github-actions bot commented Dec 7, 2020

anuraaga commented Dec 7, 2020

NathanielRN commented Dec 7, 2020

github-actions bot commented Dec 15, 2020

anuraaga commented Dec 15, 2020

NathanielRN commented Dec 15, 2020

Oberon00 Sep 25, 2020 •

edited

Loading

NathanielRN commented Sep 25, 2020 •

edited

Loading

tigrannajaryan commented Sep 25, 2020 •

edited

Loading

arminru Oct 20, 2020 •

edited

Loading