feat(basic-tracer): add recording Span implementation #102

mayurkale22 · 2019-07-12T23:14:38Z

~~This PR contains real implementation of Span interface. Currently, this class is part of core package but, ultimately it should be shifted to basic-tracer package (dependency on #99).~~

~~*Note: tests are still pending, I will add once we agreed on the initial approach/implementation.~~

packages/opentelemetry-core/src/trace/Span.ts

hekike · 2019-07-13T17:06:13Z

packages/opentelemetry-core/src/trace/Span.ts

+      this._parentId = spanContext.spanId;
+      this._traceState = spanContext.traceState;
+    } else {
+      this._traceId = randomTraceId();


is this the span's responsibility?

@hekike what's your take on this?

packages/opentelemetry-core/src/trace/Span.ts

draffensperger

Why would't we want the base Span to be in the core package? Wouldn't the automatic tracers need the same Span as the basic one?

draffensperger · 2019-07-15T01:21:12Z

packages/opentelemetry-core/src/trace/Span.ts

+  /** Constructs a new Span instance. */
+  constructor(
+    readonly _parentTracer: types.Tracer,
+    readonly _spanName: string,


Could you make this private readonly _spanName: string, to avoid needing the separate this._name = _spanName?

Not sure whether it's better called _name or _spanName

readonly is not possible as we have updateName method to update the span name.

Ah, ok. Can you say then private _spanName so that you don't need a separate _name variable?

draffensperger · 2019-07-15T01:23:26Z

packages/opentelemetry-core/src/trace/Span.ts

+      // This is a root span so no remote or local parent.
+      this._traceId = randomTraceId();
+    }
+    this._startTime = _options.startTime || performance.now();


Sweet about use of performance.now! I'm excited with the thought of making the timestamps be monotonic using this, which will lead to higher accuracy timestamps and make things easier in the browser. We should document it though so people know what to expect - and we will need some utility functions down the line to make it easy for exporters to convert it.

And maybe we should confirm this decision in the SIG - was it fully decided that we will use performance.now for timestamps?

draffensperger · 2019-07-15T01:23:46Z

packages/opentelemetry-core/src/trace/Span.ts

+    const spanContext: types.SpanContext = {
+      traceId: this._traceId,
+      spanId: this._spanId,
+      traceOptions: types.TraceOptions.SAMPLED,


Why does this specify SAMPLED here?

Also curious about this

Was a little confuse here, Is it based on the sampling decision?

Oh hm I guess this depends on the tracer's startSpan implementation. The basictracer implementation creates a noop span if the span isn't sampled, so the real span implementation here is always sampled: https://github.com/open-telemetry/opentelemetry-js/pull/99/files#diff-b4b335d4beecdee2577ffca9f9b398c7R84

@draffensperger Do you agree on this?

Ah, that makes sense. Could you just add a comment explaining that the non-sampled Spans use the NoOp span implementation, so that this will always be sampled?

Alternatively, can we have this be sent by the tracer itself so we don't need to set it here but rather do it in the constructor?

draffensperger · 2019-07-15T01:24:15Z

packages/opentelemetry-core/src/trace/Span.ts

+    try {
+      // If the value is a typeof object it has to be JSON.stringify-able
+      const serializedValue =
+        typeof value === 'object' ? JSON.stringify(value) : value;


Should this this stringify behavior be documented?

I actually think it should simply not happen. The tracer should accept any type and then it's up to the vendors to handle them. Serialization would happen in the default exporter.

Maybe we could make a shared defaultSerializer function that exporters could optionally use as well?

Sure, especially since I expect most vendors to just use strings, but we support many different types on our end.

mayurkale22 · 2019-07-15T17:53:51Z

Why would't we want the base Span to be in the core package?

I think this is an implementation of Span API and it should be part of SDK (basic-tracer). That means vendors can write their own flavor of tracer/SDK based on standard sets of clean APIs defined in the core package. Let's discuss more about this in next SIG meeting.

Wouldn't the automatic tracers need the same Span as the basic one?

I think auto-tracer SDK is based on basic-tracer and asynchooks-scope-manager, we can wrap Span and Tracer implementation in automatic tracer from basic tracer package.

bg451

looks like a good starting point

bg451 · 2019-07-15T18:07:12Z

packages/opentelemetry-core/src/trace/Span.ts

+    // @todo: record or export the span
+  }
+
+  isRecordingEvents(): boolean {


can the span be recording events if the span/trace isn't sampled? What if the span is sampled but isRecordingEvents was not set in the options?

I think the assumption is all the sampled span has isRecordingEvents flag as true.

Java's implementation: https://github.com/open-telemetry/opentelemetry-java/blob/master/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java#L179

bg451 · 2019-07-15T18:13:15Z

packages/opentelemetry-core/src/trace/Span.ts

+    const spanContext: types.SpanContext = {
+      traceId: this._traceId,
+      spanId: this._spanId,
+      traceOptions: types.TraceOptions.SAMPLED,


Also curious about this

packages/opentelemetry-core/src/trace/Span.ts

rochdev · 2019-07-16T15:03:31Z

packages/opentelemetry-core/src/trace/Span.ts

+    try {
+      // If the value is a typeof object it has to be JSON.stringify-able
+      const serializedValue =
+        typeof value === 'object' ? JSON.stringify(value) : value;


I actually think it should simply not happen. The tracer should accept any type and then it's up to the vendors to handle them. Serialization would happen in the default exporter.

packages/opentelemetry-core/src/trace/Span.ts

mayurkale22 · 2019-07-18T05:02:37Z

Why would't we want the base Span to be in the core package?

Based on SIG Discussion

Core is basic sets of APIs with Noop and default implementations.
Core is empty shelf
Other languages doing the same (API + SDK)
RealSpan: Probably it should live in basic-tracer

codecov-io · 2019-07-25T18:28:41Z

Codecov Report

Merging #102 into master will decrease coverage by 0.96%.
The diff coverage is 85.44%.

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
- Coverage   98.74%   97.78%   -0.97%     
==========================================
  Files          29       31       +2     
  Lines        1915     2210     +295     
  Branches      221      262      +41     
==========================================
+ Hits         1891     2161     +270     
- Misses         24       49      +25

Impacted Files	Coverage Δ
packages/opentelemetry-basic-tracer/src/Span.ts	`85.44% <85.44%> (ø)`
...ages/opentelemetry-basic-tracer/src/BasicTracer.ts	`98.54% <0%> (ø)`

packages/opentelemetry-basic-tracer/src/Span.ts

- Log a warning when span is already ended. - Validate spancontext - Add serialization logic for setAttribute - Add spancontext-utils

- Create SpanContext in constructor once - Rename _getSpanContext to _getParentSpanContext - Rename underscore prefix from params - no-parameter-properties

…ld happen in the default exporter)

rochdev · 2019-07-29T14:29:32Z

packages/opentelemetry-basic-tracer/src/Span.ts

+  private _name: string;
+  private _ended = false;
+  private _startTime: number;
+  private _endTime = 0;


Didn't we agree to use duration for Node specifically because it provides higher precision as a number than an end time?

Do you think that still applies now that we use performance.now for the timestamps (instead of epoch millis)?

It is true that Node processes that have been running for > ~90 days will begin to get accuracy slightly less than nanosecond accuracy for performance.now timestamps, but accuracy of 10 nanos still applies for performance.now timestamps even for processes that have been running for 2 years (try running 2*365*24*3600*1000 + 1e-5 in a console).

My opinion though is that given this relatively high accuracy even for long running jobs that we don't worry about the workaround to treat span duration differently from its start time or the timestamps of events within a span.

See #19 for some related discussion

Also, given that we would be calculating duration as the different between performance.now timestamps, it's not totally clear to me that we would get higher accuracy that route. We'd need to use something like process.hrtime, which then requires wrapping it in a platform specific code.

Moved this conversation in a separate issue #144 to unblock this PR, will revisit again based on discussion.

…r-commitlint-monorepo Update commitlint monorepo to v9 (major)

mayurkale22 requested review from danielkhan, iredelmeier, justindsmith and rochdev as code owners July 12, 2019 23:14

mayurkale22 requested a review from hekike July 12, 2019 23:14

mayurkale22 added this to the SDK complete milestone Jul 12, 2019

mayurkale22 self-assigned this Jul 12, 2019

vmarchaud reviewed Jul 13, 2019

View reviewed changes

packages/opentelemetry-core/src/trace/Span.ts Outdated Show resolved Hide resolved

hekike approved these changes Jul 13, 2019

View reviewed changes

mayurkale22 force-pushed the realSpan branch 2 times, most recently from e0f3a31 to 67e6c24 Compare July 13, 2019 19:51

draffensperger reviewed Jul 15, 2019

View reviewed changes

bg451 reviewed Jul 15, 2019

View reviewed changes

mayurkale22 force-pushed the realSpan branch 3 times, most recently from bfc54ac to 128061a Compare July 15, 2019 19:41

rochdev reviewed Jul 16, 2019

View reviewed changes

mayurkale22 force-pushed the realSpan branch from 128061a to feaa81e Compare July 16, 2019 16:59

rochdev reviewed Jul 17, 2019

View reviewed changes

packages/opentelemetry-core/src/trace/Span.ts Outdated Show resolved Hide resolved

mayurkale22 force-pushed the realSpan branch 3 times, most recently from bafec6c to 0661b73 Compare July 18, 2019 04:48

mayurkale22 mentioned this pull request Jul 19, 2019

basic-tracer initial implementation #99

Merged

rochdev approved these changes Jul 22, 2019

View reviewed changes

mayurkale22 mentioned this pull request Jul 24, 2019

feat: Add basic-tracer package #131

Merged

mayurkale22 force-pushed the realSpan branch from 2ee9d16 to 09540f5 Compare July 25, 2019 18:26

mayurkale22 changed the title ~~Add Real Span~~ feat(basic-tracer): add recording Span implementation Jul 25, 2019

draffensperger reviewed Jul 25, 2019

View reviewed changes

mayurkale22 force-pushed the realSpan branch from 82bacb5 to 763dc2b Compare July 26, 2019 17:00

draffensperger approved these changes Jul 26, 2019

View reviewed changes

packages/opentelemetry-basic-tracer/src/Span.ts Outdated Show resolved Hide resolved

mayurkale22 added 13 commits July 27, 2019 10:20

Add Real Span

0d867d1

Fix review comments

c38ac46

- Log a warning when span is already ended. - Validate spancontext - Add serialization logic for setAttribute - Add spancontext-utils

Fix review comments, fix build, add span tests

b954ae5

Fix review comments

cbe346d

- Create SpanContext in constructor once - Rename _getSpanContext to _getParentSpanContext - Rename underscore prefix from params - no-parameter-properties

Remove attribute value serialization at Span level (Serialization wou…

b121031

…ld happen in the default exporter)

Make isRecordingEvents=true for RealSpan

09e5723

merge master

d27e4f1

Add more tests

23f4506

refactor: move real Span in basic-tracer package

6eadadb

chore: add codecov script

86bc027

fix: add readonly

029b3d7

fix: store finishTime instread of duration

659282b

fix: rename _finishTime to _endTime

45d1785

mayurkale22 force-pushed the realSpan branch from 0d420cf to 45d1785 Compare July 27, 2019 17:20

rochdev reviewed Jul 29, 2019

View reviewed changes

Merge branch 'master' into realSpan

5d80e3c

mayurkale22 mentioned this pull request Jul 29, 2019

use duration for Node because it provides higher precision as a number than an end time #144

Closed

mayurkale22 merged commit 7b46da2 into open-telemetry:master Jul 29, 2019

mayurkale22 deleted the realSpan branch July 29, 2019 21:18

mayurkale22 mentioned this pull request Jul 30, 2019

Add opentelemetry/basic-tracer package and implementation #90

Closed

mayurkale22 mentioned this pull request Aug 7, 2019

Nominating @bg451 as JavaScript approver open-telemetry/community#157

Merged

mayurkale22 mentioned this pull request Sep 10, 2019

Nominating @vmarchaud as JavaScript approver open-telemetry/community#177

Merged

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023

Merge pull request open-telemetry#102 from renovate-bot/renovate/majo…

9fbb301

…r-commitlint-monorepo Update commitlint monorepo to v9 (major)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(basic-tracer): add recording Span implementation #102

feat(basic-tracer): add recording Span implementation #102

mayurkale22 commented Jul 12, 2019 •

edited

Loading

hekike Jul 13, 2019

mayurkale22 Jul 25, 2019

draffensperger left a comment

draffensperger Jul 15, 2019

mayurkale22 Jul 15, 2019

draffensperger Jul 15, 2019

draffensperger Jul 15, 2019

draffensperger Jul 15, 2019

bg451 Jul 15, 2019

mayurkale22 Jul 15, 2019

bg451 Jul 15, 2019

mayurkale22 Jul 15, 2019

draffensperger Jul 16, 2019

draffensperger Jul 15, 2019

rochdev Jul 16, 2019

draffensperger Jul 16, 2019

rochdev Jul 17, 2019

mayurkale22 commented Jul 15, 2019

bg451 left a comment

bg451 Jul 15, 2019

mayurkale22 Jul 17, 2019

bg451 Jul 15, 2019

rochdev Jul 16, 2019

mayurkale22 commented Jul 18, 2019

codecov-io commented Jul 25, 2019 •

edited

Loading

rochdev Jul 29, 2019

draffensperger Jul 29, 2019 •

edited

Loading

draffensperger Jul 29, 2019

mayurkale22 Jul 29, 2019

feat(basic-tracer): add recording Span implementation #102

feat(basic-tracer): add recording Span implementation #102

Conversation

mayurkale22 commented Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

draffensperger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Jul 15, 2019

bg451 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Jul 18, 2019

codecov-io commented Jul 25, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

draffensperger Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Jul 12, 2019 •

edited

Loading

codecov-io commented Jul 25, 2019 •

edited

Loading

draffensperger Jul 29, 2019 •

edited

Loading