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

Are IsValid and IsRemote required? #908

Closed
MrAlias opened this issue Sep 1, 2020 · 8 comments · Fixed by #914
Closed

Are IsValid and IsRemote required? #908

MrAlias opened this issue Sep 1, 2020 · 8 comments · Fixed by #914
Labels
area:api Cross language API specification issue bug Something isn't working priority:p3 Lowest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Sep 1, 2020

Currently the SpanContext section contains both IsValid and IsRemote as subsections. There is not normative statements given about if these portions of the API are required, recommended, or options as far as I can tell.

To make the specification clear about what implementations must, should, or can include this should be made explicit.

@MrAlias MrAlias added the spec:trace Related to the specification/trace directory label Sep 1, 2020
@Oberon00
Copy link
Member

Oberon00 commented Sep 1, 2020

If it's stated like a fact, it's a MUST, I'd say.

@Oberon00 Oberon00 added area:api Cross language API specification issue release:after-ga Not required before GA release, and not going to work on before GA labels Sep 1, 2020
@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 1, 2020

... I'd say.

Just based on your response alone it should be clear that assuming things will be interpreted in a particular way is subjective. Subjectivity does not have a place in a specification that is laying out directives for compliance. It is also not something found in the notation conventions this project has stated it adheres to. This issue is asking for any subjective understanding of the authors to be communicated in the specification explicitly by following this notational conventions.

I would also ask this be done prior to GA as I am still not certain if these are requirements, recommendations, or suggestions for a SIG to implement.

@Oberon00
Copy link
Member

Oberon00 commented Sep 1, 2020

I agree that this would be preferable, and feel free to create a PR to exchange every fact (not annotated with "Note:") with a MUST. But I don't think it's very urgent. Others might disagree though, so I'll remove the after-ga label and let others decide. EDIT: Let's also say this is a bug.

@Oberon00 Oberon00 added bug Something isn't working and removed release:after-ga Not required before GA release, and not going to work on before GA labels Sep 1, 2020
@arminru arminru added release:required-for-ga Must be resolved before GA release, or nice to have before GA priority:p3 Lowest priority level labels Sep 2, 2020
@arminru
Copy link
Member

arminru commented Sep 2, 2020

@open-telemetry/javascript-maintainers According to the spec compliance matrix, JS is the only implementation known to not implement IsValid. Is there any particular reason for that or was it just interpreted as optional?

@Oberon00
Copy link
Member

Oberon00 commented Sep 2, 2020

@MrAlias Do you want to open a new issue for cleaning up the whole spec for MUSTs stated as facts? This could be resolved either by adding the MUSTs or stating that every fact not annotated with Note/usually/is expected to is a MUST and checking the spec for facts that should be notes.

@dyladan
Copy link
Member

dyladan commented Sep 2, 2020

@arminru there is a PR open for it. Simply a matter of limited developer hours and other priorities.

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 2, 2020

@MrAlias Do you want to open a new issue for cleaning up the whole spec for MUSTs stated as facts? This could be resolved either by adding the MUSTs or stating that every fact not annotated with Note/usually/is expected to is a MUST and checking the spec for facts that should be notes.

I'm open to suggestions, but my initial thought would be to open issues for underspecified portions of the spec as they are identified. I'm guessing your are more familiar with the entirety of the specification and might have more insight into the total scope of areas that need these types of revisions so I would be interested to understand what your think is the appropriate course of action and what size you think the scope of this is @Oberon00.

@Oberon00
Copy link
Member

Oberon00 commented Sep 2, 2020

Personally I think we can do this on an on-demand base. So we can just close this issue with #914 then and open a new one if something particular comes up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue bug Something isn't working priority:p3 Lowest priority level 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
Development

Successfully merging a pull request may close this issue.

4 participants