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

Align entity and parameter descriptions in Trace API #850

Open
arminru opened this issue Aug 21, 2020 · 5 comments
Open

Align entity and parameter descriptions in Trace API #850

arminru opened this issue Aug 21, 2020 · 5 comments
Assignees
Labels
area:api Cross language API specification issue release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory

Comments

@arminru
Copy link
Member

arminru commented Aug 21, 2020

In specification/trace/api.md, some API definitions list parameters with Required/Optional as a prefix while others only mention this in the prose.
Furthermore, it is not always clear what's a required property of an entity in the model (i.e., always present, like the start or end timestamp of a span or an event timestamp) yet optional in the API since default values are defined (e.g., since implementations have to set the time of calling the API as a timestamp if no custom one is provided as an argument).

@arminru arminru added area:api Cross language API specification issue spec:trace Related to the specification/trace directory labels Aug 21, 2020
@arminru
Copy link
Member Author

arminru commented Aug 21, 2020

@open-telemetry/technical-committee Should we add a label "editorial" or "documentation" for issues/PRs intended to improve documentation without changing any meaning?

@carlosalberto
Copy link
Contributor

A "documentation" label sounds good to me.

@andrewhsu andrewhsu added release:required-for-ga Must be resolved before GA release, or nice to have before GA priority:p3 Lowest priority level labels Aug 21, 2020
@Oberon00
Copy link
Member

Oberon00 commented Aug 24, 2020

Furthermore, it is not always clear what's a required property of an entity in the model

I think the API spec cannot prescribe an actual entity model as in prescribing what data should actually be stored. It only describes the API. And if an API implementation decides to throw away some (or all in the case of no-op SDK) information, that should be OK.

@bogdandrutu
Copy link
Member

@Oberon00 completely agree, and we should describe what the API will "require" users to provide vs what "optionally" users can provide. This is why I am so confused about #839

@arminru
Copy link
Member Author

arminru commented Aug 26, 2020

@Oberon00 I agree that the API spec cannnot prescribe an entity model but consumers need to know what data they can expect when writing exporters, for example. Given that implementations are free to discard any data passed by users to the API, this would indeed go better into the SDK spec.

@bogdandrutu I explained extensively on #839 (comment) why I removed the inconsistent and wrong wording there:

Keeping the wording that the timestamp is an optional property of an event is wrong, however, so dropping the misleading part and opening #850 for consolidating it across the file was the best compromise to move forward IMHO.
This section describes the event itself with its non-optional timestamp and the API definition for adding events below explicitly states the timestamp is optional as a parameter to this API and that it will use the current time as a default if not provided.
We only have these (Required)/(Optional) prefixes for the Events and Links sections here and now only Links are left with it. All the other API definitions don't use this notion anyway so it's not aligned already and this change is not making anything worse. I also don't think the change is unrelated to this PR which is intended to clarify the timestamps origin and range.

To stay consistent with what we seem to agree here - that the API cannot prescribe an entity model - the mentions saying "An Event has the following properties" and the like should be removed from the spec since this does exactly that, prescribing an entity model. It should only say "There MUST be an API AddEvent accepting the following parameters ...", right?

Note that I do, however, think it is legitimate for the API spec to prescribe which values are used if optional parameters are left out, so specifying that the current time at calling the API is applied if no timestamp is provided is fine IMHO. This is something often seen in interface definitions which, for example, say calling f(a) has the same effect as calling f(a,b) with b=0. If we would leave everything to the implementation to decide, this would make it more complicated to use, as users would have to read the SDK spec to figure out which timestamp is applied if the optional one is not provided and potentially lead to non-default-SDK implementations being inconsistent.

@arminru arminru self-assigned this Sep 17, 2020
@arminru arminru added release:after-ga Not required before GA release, and not going to work on before GA and removed priority:p3 Lowest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 25, 2020
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 release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory
Projects
None yet
Development

No branches or pull requests

5 participants