-
Notifications
You must be signed in to change notification settings - Fork 440
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
Set span start and end timestamps #98
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@@ -39,7 +39,7 @@ class Span final : public trace::Span | |||
|
|||
void UpdateName(nostd::string_view name) noexcept override { span_->UpdateName(name); } | |||
|
|||
void End() noexcept override { span_->End(); } | |||
void End(const trace::EndSpanOptions &options = {}) noexcept override { span_->End(options); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we have different structures for StartSpanOptions and EndSpanOptions?
Would it make sense to have one common options class, such as SpanOptions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent behind this design is to make it very clear to the API user which option can be set at which time (at start and end).
This also reflects the OpenTracing API, which provides FinishSpanOptions and StartSpanOptions.
@pyohannes please rebase, thanks. |
@reyang This is rebased now. |
This adds SDK support for setting start and end timestamps on spans.
SystemTimestamp
andSteadyTimestamp
are added.Span
implementation in the SDK is updated to use given timestamps or calculate timestamps based on the current time.