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

Add a section for OTel specific values in TraceState. #1852

Merged
merged 30 commits into from Oct 15, 2021

Conversation

carlosalberto
Copy link
Contributor

Related to open-telemetry/oteps#168 as we want to be able to specify OTel-specific values how in tracestate (in the case of the aforementioned OTEP, as we want to store probability and a random number, for sampling purposes).

This is a draft for now, in order to get OTEP 168 make progress.

cc @yurishkuro

carlosalberto and others added 3 commits August 5, 2021 17:37
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
@carlosalberto
Copy link
Contributor Author

@yurishkuro Please review, as I've updated this PR based on your feedback ;)

specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
@carlosalberto
Copy link
Contributor Author

carlosalberto commented Aug 19, 2021

@yurishkuro @Oberon00 Updated the document to use the same notation used by the TraceContext specification. Specific changes:

  1. Mentioned the entire list MUST NOT exceed 256 characters, as the related TraceContext's value section specifies.

  2. Keys are limited to two characters, mostly being alphanumeric (p, rs or t1, for example). I think this will give OTel concerns enough range of sub-keys. Otherwise, happy to change it.

  3. Values are limited to a-z / A-Z / 0-9 / "-" / "_" / "." - I think it makes sense to start with a small range of allowed characters, but have otherwise no opinion (will cc a few people who may know more).

  4. Did NOT put a limit on the sub values, as choosing anything feels rather random - should it be 10? Should it be 6? Should it be 4? For now, I only mentioned again that 256 is the maximum length for the entire list.

cc @dyladan @mwear

specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
@carlosalberto
Copy link
Contributor Author

Hey @yurishkuro

Iterated and updated over this PR. Left a small note regarding SDKs being encouraged to discard a value update if it ends up exceeding the 256 char limit.

I would consider instead requiring API-only access, like SetInclusionProbabilityLog2(p). This may be a question for a different PR though

Agreed. Having such abstraction would be a good thing to have. I'd vow for specifying them in a follow up, once the foundation here is done (more specifically on the SetInclusionProbabilityLog2() operation (and such) operations, I imagine we defining them in their own concerns, way down the road).

A final note is that I feel now that this handling should be a SDK-wide part, more than a Trace one.

@carlosalberto
Copy link
Contributor Author

I think that question should be addressed here, not there.

Agreed, let's do that here.

@carlosalberto
Copy link
Contributor Author

Updated the PR to reflect the latest feedback:

  • Mention that ONLY keys defined in the Specification should be used - instrumentation libraries and clients MUST use their own entry.
  • Upon updates, no order needs to be preserved.

Question: Should we really go back to otel as the entry key?

@carlosalberto
Copy link
Contributor Author

Ping @bogdandrutu - I think we are ready to go, unless you think there's something we need to tune still.

specification/trace/tracestate-handling.md Show resolved Hide resolved
@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Sep 29, 2021
@carlosalberto
Copy link
Contributor Author

Ping @Oberon00 @bogdandrutu - hopefully we can iterate on the remaining bits so this can be merged ;)

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.

None yet

9 participants