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

Clarify event timestamp origin and range #839

Merged
merged 19 commits into from
Sep 17, 2020
Merged

Clarify event timestamp origin and range #839

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
bb1e233
Document that event timestamps might be out of range
arminru Aug 20, 2020
fcbc5fe
Add to changelog
arminru Aug 20, 2020
8ded9fa
Update specification/trace/api.md
arminru Aug 20, 2020
c5ff2f3
Clarify that custom timestamps should only be used when necessary
arminru Aug 20, 2020
8f17b7f
Generalize occasions when timestamps might be out of range
arminru Aug 20, 2020
d5ba3bb
Point out that a timestamp is always there but can be overriden by th…
arminru Aug 20, 2020
d0f6e62
Remove inconsistent required/optional from property list
arminru Aug 21, 2020
4b44694
Merge branch 'master' into define-timestamp-range
arminru Aug 21, 2020
a3122b2
Merge branch 'master' into define-timestamp-range
arminru Aug 24, 2020
e3db762
Merge branch 'master' into define-timestamp-range
arminru Aug 24, 2020
fbaf199
Merge branch 'master' into define-timestamp-range
arminru Aug 25, 2020
bcc1b72
Merge branch 'master' into define-timestamp-range
arminru Aug 25, 2020
e329bbe
Merge branch 'master' into define-timestamp-range
arminru Aug 27, 2020
5d779f7
Merge branch 'master' into define-timestamp-range
arminru Aug 27, 2020
62b2dcc
Merge branch 'master' into define-timestamp-range
arminru Aug 28, 2020
9b684c9
Merge branch 'master' into define-timestamp-range
arminru Sep 2, 2020
3461fa2
Merge branch 'master' into define-timestamp-range
arminru Sep 3, 2020
548b655
Merge branch 'master' into define-timestamp-range
arminru Sep 8, 2020
2c45fef
Merge branch 'master' into define-timestamp-range
arminru Sep 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ Updates:
* SpanProcessors must provide read/write access at least in OnStart.
- Specify how `Probability` sampler is used with `ParentOrElse` sampler.
- Clean up api-propagators.md, by extending documentation and removing redundant sections ([#577](https://github.com/open-telemetry/opentelemetry-specification/pull/577))
- Clarify event timestamp origin and range ([#839](https://github.com/open-telemetry/opentelemetry-specification/pull/839))

## v0.6.0 (07-01-2020)

Expand Down
18 changes: 15 additions & 3 deletions specification/trace/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,15 +428,20 @@ with the moment when they are added to the `Span`.
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
Copy link
Member

Choose a reason for hiding this comment

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

What? This is not required.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@arminru I agree. This a mandatory property of the Event object. It is an optional parameter of the API call.

Copy link
Member

@bogdandrutu bogdandrutu Aug 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

For end it is clearly stated as optional, for span creation it uses a different wording, and it says required only for the name

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please take another look @bogdandrutu 🙂

added or a custom timestamp provided by the user.
- (Optional) [`Attributes`](../common/common.md#attributes).
- (Optional) Timestamp for the event. If not provided, the current time when the event is added MUST be used.

The `Event` SHOULD be an immutable type.

The Span interface MUST provide:

- An API to record a single `Event` where the `Event` properties are passed as
arguments. This MAY be called `AddEvent`.
This API takes the name of the event, optional `Attributes` and an optional
`Timestamp` which can be used to specify the time at which the event occurred.
If no custom timestamp is provided by the user, the implementation automatically
sets the time at which this API is called on the event.
- An API to record a single `Event` whose attributes or attribute values are
lazily constructed, with the intention of avoiding unnecessary work if an event
is unused. If the language supports overloads then this SHOULD be called
Expand All @@ -445,8 +450,15 @@ The Span interface MUST provide:
wrapping class or function that returns an `Event` or formatted attributes. When
providing a wrapping class or function it SHOULD be named `EventFormatter`.

Events SHOULD preserve the order in which they're set. This will typically match
the ordering of the events' timestamps.
Events SHOULD preserve the order in which they are recorded.
This will typically match the ordering of the events' timestamps,
but events may be recorded out-of-order using custom timestamps.

Consumers should be aware that an event's timestamp might be before the start or
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.
Copy link
Member

Choose a reason for hiding this comment

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

👍


Note that the OpenTelemetry project documents certain ["standard event names and
keys"](semantic_conventions/README.md) which have prescribed semantic meanings.
Expand Down