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 9 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))
- Rename HTTPText propagator to TextMap ([#793](https://github.com/open-telemetry/opentelemetry-specification/pull/793))
- Rename ParentOrElse sampler to ParentBased and add multiple delegate samplers ([#610](https://github.com/open-telemetry/opentelemetry-specification/pull/610))
- Rename ProbabilitySampler to TraceIdRatioBasedSampler and add requirements ([#611](https://github.com/open-telemetry/opentelemetry-specification/pull/611))
Expand Down
22 changes: 17 additions & 5 deletions specification/trace/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,16 +427,21 @@ with the moment when they are added to the `Span`.

An `Event` is defined by the following properties:

- (Required) Name of the event.
- (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.
- 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.
Comment on lines +435 to +438
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned before, I think this is an unrelated change and should be made across the entire file if we want.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bogdandrutu ^^^
I think we should be able to move forward with this PR and sort out the rest on #850.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bogdandrutu ^^^
I think we should be able to move forward with this PR and sort out the rest on #850.


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