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

Update compliance matrix for removal of SpanContext or Span as parent (#510) #1079

Merged
merged 3 commits into from
Oct 13, 2020
Merged

Update compliance matrix for removal of SpanContext or Span as parent (#510) #1079

merged 3 commits into from
Oct 13, 2020

Conversation

Oberon00
Copy link
Member

@Oberon00 Oberon00 commented Oct 9, 2020

Follow-up for issue #510 (PR #875).

Changes

  • Update compliance matrix for removal of support for Span and SpanContext as explicit parents.

Related Issues/PRs

I think this should be merged before renaming SpanContext to SpanReference to avoid confusion: #1075.

@Oberon00 Oberon00 added area:api Cross language API specification issue area:span-relationships Related to span relationships spec:trace Related to the specification/trace directory release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Oct 9, 2020
@Oberon00 Oberon00 requested review from a team October 9, 2020 09:02
@Oberon00 Oberon00 mentioned this pull request Oct 9, 2020
Merged
@Oberon00 Oberon00 changed the title Update compliance matrix for #510. Update compliance matrix for removal of SpanContext or Span as parent (#510). Oct 9, 2020
@Oberon00 Oberon00 changed the title Update compliance matrix for removal of SpanContext or Span as parent (#510). Update compliance matrix for removal of SpanContext or Span as parent (#510) Oct 9, 2020
iNikem
iNikem reviewed Oct 9, 2020
View reviewed changes
spec-compliance-matrix.md Outdated
@@ -31,8 +31,7 @@ status of the feature is not known.
|Create root span | + | + | + | + | + | + | + | + | + | + |
|Create with default parent (active span) | + | + | + | + | + | + | + | + | + | + |
|Create with parent from Context | + | + | + | + | + | + | + | + | + | + |
|Create with explicit parent Span | + | + | + | + | + | + | + | - | + | + |
|Create with explicit parent SpanContext | - | + | + | + | + | + | | - | + | + |
|No explicit parent Span/SpanContext allowed | | | | | | | | | | |
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the first "negative" compliance? Where we actually verify that something is NOT done?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think so. It seems we did not update this matrix for the Statuscode removals/updates either.

Copy link
Member

Choose a reason for hiding this comment

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

It's similar to this entry in the attributes section below:

null values documented as invalid/undefined

Copy link
Member Author

@Oberon00 Oberon00 Oct 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we could leave in the old lines but add a strike-through style? Then, after some transition period, we remove it. We could also add a new cell entry type like rm to signify removal of the feature.

I think the way I did it not now is the most straightforward though.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, this is done in java, so you can pre-populated a + for it

Copy link
Member Author

Choose a reason for hiding this comment

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

@jkwatson Done: 00bafd2.

@Oberon00
Copy link
Member Author

@open-telemetry/specs-approvers: This should be an editorial/trivial change but it needs one more approval.

@Oberon00 Oberon00 marked this pull request as draft October 12, 2020 15:40
@Oberon00
Copy link
Member Author

I addded "SpanProcessor.OnStart receives parent Context" in, c0c6005. This is the first SDK-specific compliance entry as far as I can see. Do we still want it? Otherwise I can revert the commit.

@Oberon00 Oberon00 marked this pull request as ready for review October 12, 2020 15:47
@arminru
Copy link
Member

arminru commented Oct 12, 2020

@Oberon00 that should be fine. The env vars mentioned further down, for example, also just apply to SDKs.

@arminru arminru merged commit f8ff8c3 into open-telemetry:master Oct 13, 2020
@arminru arminru deleted the followup-spanparent-compliance branch October 13, 2020 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkwatson jkwatson jkwatson left review comments

@iNikem iNikem iNikem left review comments

@arminru arminru arminru approved these changes

@jmacd jmacd jmacd approved these changes

Assignees

@tigrannajaryan tigrannajaryan

Labels
area:api Cross language API specification issue area:span-relationships Related to span relationships release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Oberon00 @arminru @jkwatson @iNikem @jmacd @tigrannajaryan