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

Clarify that IsValid and IsRemote are APIs on SpanContext #771

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

bogdandrutu
Copy link
Member

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

Updates #753

Changes

Clarifies that IsValid and IsRemote are APIs.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu bogdandrutu requested review from a team as code owners August 10, 2020 18:02
@bogdandrutu bogdandrutu added the spec:trace Related to the specification/trace directory label Aug 10, 2020
@@ -14,6 +14,8 @@ Table of Contents
* [Tracer](#tracer)
* [Tracer operations](#tracer-operations)
* [SpanContext](#spancontext)
* [IsValid](#isvalid)
Copy link
Member

@Oberon00 Oberon00 Aug 11, 2020

Choose a reason for hiding this comment

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

I know I missed that we had IsValid (#753), but I think this was my mistake. It is actually clear enough in the spec already IMHO. If we add headings for IsValid and IsRemote, we should also add headings for SpanId, TraceId, TraceFlags, TraceState, and that would be a lot of headings.

Copy link
Member Author

Choose a reason for hiding this comment

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

They will come because there is an issue about format of trace id / span id formats that we expose. Tried to keep PRs small :)

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

I'm approving because I don't want to have this hanging in the air and I don't think it's making the spec worse, but I think the clarification is not really needed either.

@bogdandrutu
Copy link
Member Author

Trivial change, don't have to wait 48h :)

@bogdandrutu bogdandrutu merged commit 090296f into open-telemetry:master Aug 11, 2020
@bogdandrutu bogdandrutu deleted the spanids branch August 11, 2020 16:09
@obecny
Copy link
Member

obecny commented Aug 24, 2020

I think that this information (isValid) is kind of duplicate and redundant. if this "api" is meant to return true only if spanId and traceId are valid what is really the purpose of keeping this extra information on context - this sounds for me like really redundant thing. This information can be simply checked by some helper function so no reason to keep it on context. WDYT ?

@arminru
Copy link
Member

arminru commented Aug 25, 2020

@obecny The spec does not specify how it should be implemented but I'm pretty sure the intended way is indeed exposing a helper function checking spanId and traceID just as you mentioned rather than storing an actual flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants