basic-tracer initial implementation #99

hekike · 2019-07-12T01:21:00Z

It is challenging to build a minimal tracer that makes sense without propagation and exporter interfaces. Here is my initial take on it.

hekike · 2019-07-12T01:28:49Z

My GitHub is linked to a CNCF account I don't have anymore, so fixing the CLA will take some time :P

mayurkale22

This is a good start, added a few comments in the first iteration.

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

.gitignore

packages/opentelemetry-core/package.json

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

draffensperger · 2019-07-12T12:42:46Z

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

+export class Tracer implements types.Tracer {
+  static INVALID_ID = '0';
+
+  private logger!: types.Logger;


Given that this is initialized in the constructor, can we remove the ! here?

hekike · 2019-07-12T15:17:14Z

@mayurkale22 @draffensperger thanks for the review. I addressed your comments.

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

rochdev · 2019-07-12T15:41:45Z

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

+    fn: T
+  ): ReturnType<T> {
+    if (!this.scopeManager) {
+      this.logger.warn('withSpan(...) has no effect without a scopeManager');


I don't think there should be a warning for this as it's a perfectly valid use case, especially if you use the low level construct.

My thinking was that base tracer has no ScopeManager by default so calling ScopeManager related functions like withSpan is not valid. As it won't do what you would expect. To suppress the warning the user can pass a NoopScopeManager. I think it's okay as in the automatic tracer we will ship OT with CLSScopeManager and ZoneScopeManager, so this warning is really for people who try to do something custom, and I believe for them it's quite useful.

Part of what I'm hoping for from the browser side is code simplicity (as in fewer total bytes of code shipped to the client), so if we can find a way to avoid null checks and warning messages by designing the interface a bit differently I'd prefer that.

What if we had something like new NoopScopeManager({warn: true}) by default that would write warning logs any time someone attempts to call one of its methods? That way in the browser, where we use something like a GlobalWindowScopeManager the code for the if statements and warning logs can get tree shaken out (at least in principle).

@hekike
Since NoopScopeManager will have the same behavior as the current one (just calling fn, it almost cost nothing to use it and avoid null check everywhere. If you think warning user is important then a option like @draffensperger suggested would be the way to go for me.

rochdev · 2019-07-12T15:43:09Z

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

+      this.logger.warn(
+        'getCurrentSpan() returns an invalid default span without a scopeManager'
+      );
+      return BasicTracer.defaultSpan;


I think this should be null. In the noop it makes sense to always return a span so that nothing breaks when you disable the tracer, but for the base implementation null makes more sense to indicate the absence of a span on the scope.

What is the condition where we won't have a scope manager set? If we require that in the constructor, can we just delegate the default span behavior to the scope manager?

I would prefer to avoid returning null because then the TS type needs to be types.Span|null, which is annoying for other TypeScript code to consume because it requires null checks. And the null type could also be confusing to end users who would expect it to return non-null.

See my previous comment (#99 (comment)) about the default scope manager.
Looks like there are two opinions here: return null, log warning. Both can work for getCurrentSpan() and although I like the idea of null to indicate you really have no ScopeManager, but the same strict check concept is not applicable for withSpan so I'd vote for warning log. I guess throwing in withSpan without ScopeManager would be quite abusive.

Added to SIG agenda.

Actually i think it's also applicable to withSpan when using the NoopScopeManager, it will just always return a null active scope, which is fine (at least for me).

we have users that explicitly don't want scope management, and they definitely don't want a warning because of it.

Do you know what would be a use-case that you don't want scope management but you still want to call withSpan and getCurrentSpan however you know they don't do anything?

(the default null without warning for getCurrentSpan would make sense for me as it shows well that you cannot use this feature without scope manager, although the withSpan would just silently swallow this fact, I don't think you can enforce it on API level, except if you throw)

Do you know what would be a use-case that you don't want scope management but you still want to call withSpan and getCurrentSpan however you know they don't do anything?

The auto instrumentation will call these methods.

Isn't ScopeManager a hard requirement for auto instrumentation? Otherwise, you don't have a root span. How would it work?

That's a good point. Hopefully it will be possible to have at least some level of auto instrumentation even without a scope manager, but it would definitely be challenging. How would the auto instrumentation detect the presence of a proper scope manager?

rochdev · 2019-07-12T15:43:48Z

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

+
+    // make sampling decision
+    if (!this.sampler.shouldSample(parentSpanContext)) {
+      return BasicTracer.defaultSpan;


This will not propagate the trace parent and trace state properly.

I think even if sampling decision is false, we should assign correct SpanContext for DefaultSpan/NoopSpan? @rochdev this is what you meant, right?

I mean that we shouldn't return a single global instance, otherwise it will not be possible to attach the IDs on it for propagation.

rochdev · 2019-07-12T15:45:57Z

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

+    // parent is a SpanContext
+    if (
+      options.parent &&
+      typeof (options.parent as types.SpanContext).traceId


How is this converted to JavaScript? It doesn't look like this check is JS compatible. Same for other occurrences of as.

My understanding is that it's attempting to check whether the options.parent has a traceId field. Is that correct?

as is just used to force the type of a variable, in this case it would be transpiled to if (options.parent && options.parent.traceId) {}

Can we remove the typeof then?

could someone show me an example of how would it work with TS?
I get a Type error without typeof, instanceof also doesn't work.

What error do you get if you remove the typeof?

Okay, I see now. The typeof can be removed, the as not.

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

draffensperger · 2019-07-12T12:47:00Z

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

+   */
+  getCurrentSpan(): types.Span {
+    // Return with defaultSpan if no scope manager provided.
+    if (!this.scopeManager) {


In what case could this.scopeManager be falsey? Isn't it set in the constructor? Or is this attempting to guard against someone changing it to undefined after the initial tracer construction?

I think once we set to default no-op ScopeManager in constructor (this.scopeManager = config.scopeManager || new NoopScopeManager()), this check can be removed.

draffensperger · 2019-07-12T16:46:43Z

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

+    // parent is a SpanContext
+    if (
+      options.parent &&
+      typeof (options.parent as types.SpanContext).traceId


My understanding is that it's attempting to check whether the options.parent has a traceId field. Is that correct?

draffensperger · 2019-07-12T16:59:53Z

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

+      this.logger.warn(
+        'getCurrentSpan() returns an invalid default span without a scopeManager'
+      );
+      return BasicTracer.defaultSpan;


What is the condition where we won't have a scope manager set? If we require that in the constructor, can we just delegate the default span behavior to the scope manager?

I would prefer to avoid returning null because then the TS type needs to be types.Span|null, which is annoying for other TypeScript code to consume because it requires null checks. And the null type could also be confusing to end users who would expect it to return non-null.

mayurkale22 · 2019-07-12T18:46:03Z

@hekike Please rebase and add -p ../../ to codecov based on #101.

packages/opentelemetry-basic-tracer/package.json

codecov-io · 2019-07-13T15:02:52Z

Codecov Report

Merging #99 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #99   +/-   ##
=======================================
  Coverage   98.74%   98.74%           
=======================================
  Files          29       29           
  Lines        1915     1915           
  Branches      221      221           
=======================================
  Hits         1891     1891           
  Misses         24       24

Impacted Files	Coverage Δ
...kages/opentelemetry-scope-async-hooks/src/index.ts	`100% <0%> (ø)`	⬆️
packages/opentelemetry-scope-base/src/index.ts	`100% <0%> (ø)`	⬆️
packages/opentelemetry-scope-base/src/types.ts	`100% <0%> (ø)`	⬆️
...ry-scope-async-hooks/src/AsyncHooksScopeManager.ts	`96.49% <0%> (ø)`	⬆️
...s/opentelemetry-scope-base/src/NoopScopeManager.ts	`100% <0%> (ø)`	⬆️

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

packages/opentelemetry-core/src/index.ts

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

mayurkale22

Overall looks good to me, added few last comments.

packages/opentelemetry-basic-tracer/test/BasicTracer.test.ts

mayurkale22 · 2019-07-18T21:38:45Z

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

+
+    // make sampling decision
+    if (!this.sampler.shouldSample(parentSpanContext)) {
+      return BasicTracer.defaultSpan;


I think even if sampling decision is false, we should assign correct SpanContext for DefaultSpan/NoopSpan? @rochdev this is what you meant, right?

hekike · 2019-07-19T01:55:01Z

@mayurkale22 so we would only use NoopSpan in the NoopTracer? Or the NoopSpan would accept SpanContext? How would we keep the allocation pattern low?

mayurkale22 · 2019-07-19T18:12:31Z

@mayurkale22 so we would only use NoopSpan in the NoopTracer? Or the NoopSpan would accept SpanContext? How would we keep the allocation pattern low?

This is my take, feel free to suggest if anything is wrong :)

Current Span types:
NoopSpan: all the operations are no-op except context propagation.
Span: actual implementation of Span interface, basically everything (real operations + context propagation) is possible. #102

Problem:
Although NoopSpan is lightweight in nature, but it is still allocated and some users it's not acceptable.
How would we keep the allocation pattern low? So need some solution to cover three scenarios

Singleton Span: Approx. Zero allocation, all operations are no-op.
NoRecording Span: All operations are no-op except context propagation.
Span: All real operations + context propagation.

Solution 1:
Use three different spans:
NoopSpan - Singleton, all the operations are no-op.
NoRecordingSpan - all the operations are no-op except context propagation.
RecordingSpan - all real operations.

Solution 2:
Use two spans:
NoopSpan - Singleton, all the operations are no-op.
Span - this can handle NoRecording and Recording scenarios. But have to do if (...) else condition check on every operations based on whether Span is recording or no-recording.

Solution 3:
Use two spans:
NoopSpan - Create and use static instance with invalid SpanContext (for zero allocation - export const SingletonNoopSpan = new NoopSpan(INVALID_SPAN_CONTEXT)) and have another method to create new instance of NoopSpan based on passed SpanContext. (This is based on Java's implementation).
Span - All real operations + context propagation.

hekike · 2019-07-19T23:27:59Z

I would prefer Solution 1 where NoRecordingSpan could be maybe inherited from NoopSpan.

hekike · 2019-07-19T23:29:07Z

Should we wait for this decision or should we merge this PR and fix later with a todo comment?

mayurkale22 · 2019-07-20T00:00:53Z

Should we wait for this decision or should we merge this PR and fix later with a todo comment?

I would suggest to open an issue and add @todo with the issue link to address later. This merge would unblock real Span (#102) and Automatic-Tracer implementation (#91) work.

packages/opentelemetry-types/src/trace/tracer.ts

hekike · 2019-07-26T01:40:56Z

@mayurkale22 I addressed all the comments (or added a todo) and rebased with the master. I think it's good to go.

hekike requested review from danielkhan, iredelmeier, justindsmith, mayurkale22 and rochdev as code owners July 12, 2019 01:21

mayurkale22 reviewed Jul 12, 2019

View reviewed changes

draffensperger reviewed Jul 12, 2019

View reviewed changes

rochdev reviewed Jul 12, 2019

View reviewed changes

draffensperger reviewed Jul 12, 2019

View reviewed changes

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

mayurkale22 mentioned this pull request Jul 12, 2019

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

Merged

markwolff reviewed Jul 12, 2019

View reviewed changes

packages/opentelemetry-basic-tracer/package.json Outdated Show resolved Hide resolved

hekike mentioned this pull request Jul 13, 2019

Add no-op implementations of Tracer API #98

Merged

hekike changed the title ~~feat(tracer): initial implementation~~ basic-tracer initial implementation Jul 14, 2019

mayurkale22 reviewed Jul 16, 2019

View reviewed changes

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

hekike force-pushed the feat/tracer-initial branch from b91c1c9 to e01a1e7 Compare July 17, 2019 14:38

mayurkale22 reviewed Jul 17, 2019

View reviewed changes

hekike force-pushed the feat/tracer-initial branch from 6d80a3d to 5900edf Compare July 18, 2019 02:26

mayurkale22 reviewed Jul 18, 2019

View reviewed changes

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

mayurkale22 reviewed Jul 18, 2019

View reviewed changes

mayurkale22 approved these changes Jul 20, 2019

View reviewed changes

mayurkale22 reviewed Jul 25, 2019

View reviewed changes

packages/opentelemetry-types/src/trace/tracer.ts Outdated Show resolved Hide resolved

draffensperger approved these changes Jul 25, 2019

View reviewed changes

mayurkale22 approved these changes Jul 25, 2019

View reviewed changes

mayurkale22 requested a review from rochdev July 25, 2019 20:46

Peter Marton added 16 commits July 25, 2019 18:39

feat(tracer): initial implementation

6081ff1

fix(BasicTracer): address PR comments

9fe74cc

feat(basic-tracer): move BasicTracr to a separate package

f667338

refactor(core): cleanup

4db8280

refactor(types): cleanup

5ab2f6b

feat(basic-tracer): make never sampler the default

aed4f03

docs(basic-tracer): add todo comments

830bedf

refactor(basic-tracer): prefix privates with _

5ad63f4

refactor(basic-tracer): remove typeof

1c81501

feat(basic-tracer): add binary and http text format

b9b230e

feat(basic-tracer): make scope mamnager required

41112dd

refactor(basic-tracer): use INVALID_SPAN_CONTEXT

ba2e425

docs(basic-tracer): add todo comment

424552a

fix(tracer): make ALWAYS_SAMPLER the default sampler

324bfce

fix(types): add scope-base as dependency

f649f41

refactor(tracer): address PR comments

a8b7da6

hekike force-pushed the feat/tracer-initial branch from ea92a32 to a8b7da6 Compare July 26, 2019 01:40

mayurkale22 mentioned this pull request Jul 26, 2019

Create exported const of NoopSpan instance #137

Closed

mayurkale22 merged commit 775f0e3 into open-telemetry:master Jul 26, 2019

mayurkale22 mentioned this pull request Jul 30, 2019

Add opentelemetry/basic-tracer package and implementation #90

Closed

mayurkale22 mentioned this pull request Sep 10, 2019

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

Merged

dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022

chore: fix link to API docs (open-telemetry#99)

941f58f

dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022

chore: fix link to API docs (open-telemetry#99)

4aa44e5

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

Merge pull request open-telemetry#99 from naseemkullah/fix-readme

0b313a9

basic-tracer initial implementation #99

basic-tracer initial implementation #99

Conversation

hekike commented Jul 12, 2019 • edited Loading

hekike commented Jul 12, 2019

mayurkale22 left a comment

Choose a reason for hiding this comment

draffensperger Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

hekike commented Jul 12, 2019

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

hekike Jul 13, 2019 • edited Loading

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

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 12, 2019

codecov-io commented Jul 13, 2019 • edited Loading

Codecov Report

mayurkale22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hekike commented Jul 19, 2019

mayurkale22 commented Jul 19, 2019

hekike commented Jul 19, 2019

hekike commented Jul 19, 2019

mayurkale22 commented Jul 20, 2019

hekike commented Jul 26, 2019 • edited Loading

hekike commented Jul 12, 2019 •

edited

Loading

draffensperger Jul 12, 2019 •

edited

Loading

hekike Jul 13, 2019 •

edited

Loading

codecov-io commented Jul 13, 2019 •

edited

Loading

hekike commented Jul 26, 2019 •

edited

Loading