Clarify event timestamp origin and range#839
Conversation
Oberon00
left a comment
Oberon00
left a comment
There was a problem hiding this comment.
I think we should also add a recommendation for API users to not use custom timestamps unless required.
Co-authored-by: Christian Neumüller <christian.neumueller@dynatrace.com>
arminru
changed the title
carlosalberto
added
spec:trace
| after the end of the span if custom timestamps were provided by the user for the | ||
| event or when starting or ending the span. | ||
| The specification does not require any normalization if provided timestamps are | ||
| out of range. |
bogdandrutu
left a comment
bogdandrutu
left a comment
There was a problem hiding this comment.
This PR makes timestamp required which should not be
specification/trace/api.md
Outdated
| An `Event` is defined by the following properties: | ||
|
|
||
| - (Required) Name of the event. | ||
| - (Required) A timestamp for the event. Either the time at which the event was |
There was a problem hiding this comment.
What? This is not required.
There was a problem hiding this comment.
See #839 (comment).
A timestamp is always there from an exporter's and subsequent consumer's POV. An event is defined by these properties and since it's always there it can be seen as a required part of an event.
I clarified further below that the timestamp passed to AddEvent is optional and used to override the timestamp which would otherwise be automatically set by the implementation. There is, however, no case in which there would be no timestamp for an event and therefore it's not optional.
There was a problem hiding this comment.
The first consumer of the API is not the exporter/vendor is the dev that instruments an app, so I think the specs should be written with that in mind.
Having an optional component with a default value is equivalent for the exporter to be "required".
My concern of calling this required stands, so will not approve this change.
There was a problem hiding this comment.
@arminru I agree. This a mandatory property of the Event object. It is an optional parameter of the API call.
There was a problem hiding this comment.
@tigrannajaryan sure, but the main consumer of the API is the instrumentation dev, which should not read that it is required to pass a Timestamp and as I suggested having an optional field with a default value is equivalent to always present on the consume side.
And I am less worried about some vendors not understanding this, than instrumentation dev understanding that timestamp is required.
There was a problem hiding this comment.
For end it is clearly stated as optional, for span creation it uses a different wording, and it says required only for the name
There was a problem hiding this comment.
@arminru please feel free to either align the wording to match the rest of the spec or propose a future PR which possibly goes over all places where we use "optional"/"required" qualifiers and make sure it is uniform.
Perhaps to make it easier to merge this PR you can just drop this particular change, since I believe your other clarifications later in the document regarding timestamp orders, ranges, etc are important and we can merge them.
There was a problem hiding this comment.
Strongly encourage to revert that change, and if anyone feels the need to change everywhere the wording then we should do that in a separate PR.
There was a problem hiding this comment.
I removed the (Required)/(Optional) prefix here since both would be confusing because the timestamp is not an optional property of an event but optional as an argument to the API call defined below. I filed #850 for having the notation unified across the whole file.
| - Name of the event. | ||
| - A timestamp for the event. Either the time at which the event was | ||
| added or a custom timestamp provided by the user. | ||
| - [`Attributes`](../common/common.md#attributes) further describing the event. |
There was a problem hiding this comment.
As mentioned before, I think this is an unrelated change and should be made across the entire file if we want.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@bogdandrutu ^^^
I think we should be able to move forward with this PR and sort out the rest on #850.
There was a problem hiding this comment.
@bogdandrutu ^^^
I think we should be able to move forward with this PR and sort out the rest on #850.
|
It seems to me that TC members are 2:1 for this PR (plus @arminru is a TC member himself). But maybe this is not important enough to make a vote? @open-telemetry/technical-committee @bogdandrutu either vote whether we should include this PR or decide that @bogdandrutu's opposition is accepted and the PR should be closed. |
|
So maybe it would be better to fix the consistency (i.e,. do #850) before this PR? |
|
@bogdandrutu I think @Oberon00's comment was primarily intended to indicate this PR is stuck after changes were suggested but despite these comments being addressed in August, no reply was given for more than three weeks and thus blocking the PR. Sure, that's what I created #850 for and I just assigned it to me. |
|
@arminru you have the power, you can merge this :) |
7 participants
Resolves #798