Add explicit timestamp support for Span start/end. by carlosalberto · Pull Request #571 · open-telemetry/opentelemetry-java

carlosalberto · 2019-09-27T03:43:11Z

A few notes:

Both end(Timestamp) and setStartTimestamp(Timestamp) will report an error in case the values are null. This was done to keep uniformity with the rest of the API (although internally we are handling this case).
A new TimestampConverter is created for Spans with explicit start time, and it's based on the aforementioned start time value.
Internally (SDK) I'm creating/passing around io.grpc.protobuf.Timestamp instances for the explicit values, if any. TimestampConverter already requires one, anyway. Also, we get 'free' validation of the seconds/nanos.
- HOWEVER, I'm not doing start/end timestamps comparison nor validation.

Fixes #24

carlosalberto · 2019-09-27T03:43:49Z

Also fixes #545

jkwatson · 2019-09-27T18:17:06Z

I think this is a good change. At some point in the future, it might be nice to not have the TimestampConverter us protobufs internally, but that can definitely wait.

codefromthecrypt · 2019-09-28T03:52:37Z

one hazard is allowing SDK be a dependency kitchen sink. I would recommend making strict dependency review a gate for any public release as it will cause problems for people who pull in OTel even if they dont use it.

bogdandrutu · 2019-09-28T14:15:28Z

@adriancole thanks for the comment. The main idea is that only the API artifact will be used as dependency in third-party libraries, and we try hard to limit the dependencies there.

bogdandrutu

Please split this PR into 2:

Makes the API changes and with no implementation on the SDK (no-op); The API changes looks good in general.
Changes the SDK. The timestamp logic does not seem correct to me and I would like to talk about that in more details;

bogdandrutu · 2019-09-28T14:47:05Z

+   * @param endTimestamp the explicit end {@link Timestamp} for this {@code Span}.
+   * @since 0.1.0
+   */
+  void end(Timestamp endTimestamp);


To avoid breaking people in the future, should we go with an option pattern here? That will let us add more properties in the future without changing this.

Not 100% convinced but want to hear options.

Sure, going with an option pattern sounds good to me. What about EndOptions, similar to what OC has? ;)

bogdandrutu · 2019-09-28T14:49:13Z

    Builder setSpanKind(Span.Kind spanKind);

+    /**
+     * Sets an explicit start {@link Timestamp} for the newly created {@code Span}.


Null is not accepted from what I see, then we should explain to the users:

when to set this.

what to do when this is not needed.

See your point. Will add extra clarification on this.

I really want to discourage the following use patterns:

Timestamp now = Timestamp.fromMillis(System.currentMillis()); tracer.spanBuilder().setStartTime(now).startSpan();

OR

tracer.spanBuilder().setStartTime(Timestamp.fromMillis(System.currentMillis())).startSpan();

Can you please phrase this in a way to make sure we discourage the examples I gave to you?

What is the reason for discouraging this usage?

There are few reasons here:

Java until java9 has only millis resolution (even java.time.Instant has only millis until java9).

We do want to be able to calculate all the events (in all the spans in one trace) using nanoTime relative to only one TimeConvertor (which is read only for the first Span of this trace in this process) so we can guarantee ordering of events. When using an absolute timestamp like java Instant or system.currentTimeMillis these are not monotonic so events are not order correctly.

Thanks! I suspected it was along these lines. Thanks for confirming.

Probably same comment should be for all the methods that allow startTime.

Yes (I know that we will also be adding explicit start time for Events)

codefromthecrypt · 2019-09-29T02:56:15Z

cool I will raise another issue then as the hard dep on grpc-context in the api package can easily leak implementation and/or pin deps https://github.com/open-telemetry/opentelemetry-java/blob/50a2db42b257cb9618131bffc2fbc997785b7f38/api/src/main/java/io/opentelemetry/distributedcontext/package-info.java probably to really be an api you should move grpc integration to another jar as there are alternative and arguably better context integrations. there shouldn't be a favorite that pins a dep to grpc in other words. i will copy this text to the new issue

…

On Sat, Sep 28, 2019, 10:15 PM Bogdan Drutu ***@***.***> wrote: @adriancole <https://github.com/adriancole> thanks for the comment. The main idea is that only the API artifact will be used as dependency in third-party libraries, and we try hard to limit the dependencies there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#571?email_source=notifications&email_token=AAAPVV3VICGR7P2DLGL4XILQL5RIBA5CNFSM4I3BNMFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD722T7A#issuecomment-536193532>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAPVV3UC75GCS6D2NV2DL3QL5RIBANCNFSM4I3BNMFA> .

codecov-io · 2019-09-30T00:41:12Z

Codecov Report

❌ Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@9a6c1d6). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java	0.00%	3 Missing ⚠️
...ava/io/opentelemetry/sdk/trace/SpanBuilderSdk.java	0.00%	2 Missing ⚠️

❌ Your patch status has failed because the patch coverage (64.28%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #571   +/-   ##
=========================================
  Coverage          ?   78.19%           
  Complexity        ?      686           
=========================================
  Files             ?       83           
  Lines             ?     2444           
  Branches          ?      244           
=========================================
  Hits              ?     1911           
  Misses            ?      443           
  Partials          ?       90

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

carlosalberto · 2019-10-02T02:44:40Z

Hey @bogdandrutu

I've updated the PR to not actually touch the SDK logic (as you suggested discussing the details for that in another PR). I've added a EndSpanOptions class (similar to what OC has) to contain an optional, nullable end Timestamp.

Things of interest:

Timestamp is an actual class. At first I had it as an interface, so vendors could extend it, but it feels like an overkill for such a small container.
Felt like adding a convenience create(Timestamp endTimestamp) method, so users don't have to create a builder when only setting this value, but didn't include it for cleanliness reasons.

Let me know ;)

bogdandrutu · 2019-10-02T03:19:28Z

    Builder setSpanKind(Span.Kind spanKind);

+    /**
+     * Sets an explicit start {@link Timestamp} for the newly created {@code Span}.


I really want to discourage the following use patterns:

Timestamp now = Timestamp.fromMillis(System.currentMillis()); tracer.spanBuilder().setStartTime(now).startSpan();

OR

tracer.spanBuilder().setStartTime(Timestamp.fromMillis(System.currentMillis())).startSpan();

Can you please phrase this in a way to make sure we discourage the examples I gave to you?

bogdandrutu · 2019-10-05T04:44:49Z

+   *
+   * @since 0.1
+   */
+  public static EndSpanOptions getDefault() {


Not sure this is needed for the public API. Let's remove until someone asks for it.

Works for me.

bogdandrutu · 2019-10-05T04:47:34Z

    Builder setSpanKind(Span.Kind spanKind);

+    /**
+     * Sets an explicit start {@link Timestamp} for the newly created {@code Span}.


Probably same comment should be for all the methods that allow startTime.

carlosalberto · 2019-10-06T21:35:48Z

hey @bogdandrutu I've updated the code (making EndSpanOptions.getDefault() private). Let me know if this is good for you to merge (as discussed in the Java SIG last Friday).

carlosalberto added 2 commits September 27, 2019 05:21

Allow explicit Timestamp values for Span start/end.

687e6ce

Merge branch 'master' into add_tstamp_span_life

60cd34c

carlosalberto requested review from arminru, bogdandrutu, pavolloffay, songy23 and tylerbenson as code owners September 27, 2019 03:43

jkwatson approved these changes Sep 27, 2019

View reviewed changes

bogdandrutu reviewed Sep 28, 2019

View reviewed changes

codefromthecrypt mentioned this pull request Sep 29, 2019

remove grpc-context dependency from the api package #575

Closed

Update the code after merging master.

2278c23

carlosalberto force-pushed the add_tstamp_span_life branch from 8b2455d to 2278c23 Compare September 30, 2019 00:38

Set end/start options only for the API.

8148304

carlosalberto force-pushed the add_tstamp_span_life branch from 3aa7af6 to 8148304 Compare October 2, 2019 02:40

Add a clarification on setStartTimestamp().

35d7aa8

bogdandrutu approved these changes Oct 2, 2019

View reviewed changes

arminru approved these changes Oct 2, 2019

View reviewed changes

bogdandrutu reviewed Oct 5, 2019

View reviewed changes

Make EndSpanOptions.getDefault() package private for now.

e864973

bogdandrutu merged commit 0612372 into open-telemetry:master Oct 11, 2019

kbrockhoff mentioned this pull request Oct 29, 2019

Expose Duration by Completed Span #521

Closed

Conversation

carlosalberto commented Sep 27, 2019

Uh oh!

carlosalberto commented Sep 27, 2019

Uh oh!

jkwatson commented Sep 27, 2019

Uh oh!

codefromthecrypt commented Sep 28, 2019

Uh oh!

bogdandrutu commented Sep 28, 2019

Uh oh!

bogdandrutu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt commented Sep 29, 2019 via email

Uh oh!

codecov-io commented Sep 30, 2019 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

carlosalberto commented Oct 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosalberto commented Oct 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Sep 30, 2019 •

edited by codecov Bot

Loading