feature(trace): adds named tracer factory #420

bg451 · 2019-10-10T21:08:44Z

Which problem is this PR solving?

Implements named tracers for Implement Named Tracers #403
TODO: update examples after merging in changes from feat(trace): named tracer factory prototype 2 #434

Short description of the changes

This PR adds in the TracerFactory type and updates the core's global tracer to use a tracerfactory instead of a tracer. Also modifies the TracerDelegate to be for a factory. Adds in the respective implementations for each tracer implementation.

Should name be a mandatory field in TS def? According to the specification an empty or null string can be passed as the name.

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

packages/opentelemetry-tracing/src/BasicTracerFactory.ts

Co-Authored-By: Mayur Kale <[email protected]>

…try-node into bg/add_tracer_factory_1

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

packages/opentelemetry-node/src/NodeTracerFactory.ts

dyladan

My biggest question is that there are 3 methods of getting a tracer factory:

new them directly
call static .instance method
call exported getTracerFactory function

It is not clear to me which is the best or recommended way to get a tracer factory. If the answer is not 1, then should we disallow it by making the constructor private?

dyladan · 2019-10-24T13:27:48Z

examples/basic-tracer-node/index.js

@@ -17,14 +17,14 @@ if (EXPORTER.toLowerCase().startsWith('z')) {
  exporter = new JaegerExporter(options);
 }

-const tracer = new BasicTracer();
+// Initialize the OpenTelemetry APIs to use the BasicTracer bindings
+opentelemetry.initGlobalTracerFactory(new BasicTracerFactory());


Should this use BasicTracerFactory.instance() instead of directly calling new?

IMO this (initialization of global tracer factory and BasicTracerFactory) seems a little burdensome for the end users. I was thinking something like this:

// In case of BasicTracer const opentelemetry = require('@opentelemetry/tracing'); opentelemetry.getTracerFactory().getTracer(); // should return the default BasicTracer. opentelemetry.getTracerFactory().getTracer('mongodb'); // with name opentelemetry.getTracerFactory().getTracer('redis', '1.0.0'); // with name and version opentelemetry.getTracerFactory().addSpanProcessor(...); // In case of NodeTracer const opentelemetry = require('@opentelemetry/node'); opentelemetry.getTracerFactory().getTracer(); // should return the default NodeTracer. // In case of WebTracer const opentelemetry = require('@opentelemetry/web'); opentelemetry.getTracerFactory().getTracer(); // should return the default WebTracer.

The getTracerFactory function is responsible for returning the corresponding TracerFactory and we should create (and assign) the TracerFactory instance statically once you load the library (maybe in index.ts). WDYT?

dyladan · 2019-10-24T13:28:57Z

examples/grpc/setup.js

 const { SimpleSpanProcessor } = require('@opentelemetry/tracing');
 const { JaegerExporter } = require('@opentelemetry/exporter-jaeger');
 const { ZipkinExporter } = require('@opentelemetry/exporter-zipkin');
 const EXPORTER = process.env.EXPORTER || '';

 function setupTracerAndExporters(service) {
-  const tracer = new NodeTracer({
+  const factory = new NodeTracerFactory({


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

packages/opentelemetry-node/README.md

packages/opentelemetry-tracing/src/BasicTracerFactory.ts

dyladan · 2019-10-24T13:50:46Z

packages/opentelemetry-tracing/src/BasicTracerFactory.ts

+  private _spanProcessors: SpanProcessor[] = [];
+  private _config?: BasicTracerConfig;
+
+  constructor(config?: BasicTracerConfig) {


constructor could be private, forcing users to use BasicTracerFactory.instance which would prevent creation of multiple NodeTracerFactories. Is there a use case where we would want to allow the creation of multiple or should it always be a singleton?

Just commented below but check this out: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#runtimes-with-multiple-deploymentsapplications

There definitely is a case for multiple factories.

dyladan · 2019-10-25T16:29:35Z

From #434

The duplication between BasicTracerFactory and NodeTracerFactory seems like a good candidate for inheritance

class AbstractTracerFactory<T extends types.Tracer> implements types.TracerFactory {
  private static _singletonInstance: types.TracerFactory;
  private readonly _tracers: Map<String, T> = new Map();

  protected abstract create(): T;

  getTracer(name: string = "", version?: string): T {
    const key = name + (version != undefined ? version : "");
    if (this._tracers.has(key)) return this._tracers.get(key)!;

    const tracer = this.create();
    this._tracers.set(key, tracer);
    return tracer;
  }

  static get instance(): types.TracerFactory {
    return this._singletonInstance || (this._singletonInstance = new this());
  }
}

class NodeTracerFactory extends AbstractTracerFactory<NodeTracer> {
  protected create() {
    return new NodeTracer();
  }
}

class BasicTracerFactory extends AbstractTracerFactory<BasicTracer> {
  protected create() {
    return new BasicTracer();
  }
}

Co-Authored-By: dyladan <[email protected]>

bg451 · 2019-10-30T14:50:04Z

I'm having second thoughts on the singleton approach for the factory. There is a use case for having multiple factories if your runtime contains different applications and needs to be provided different trace factory configurations. The specification mentions delegating to a different provider for this use case but honestly that's just another layer of abstraction, it would be simpler imo to just have users do new NodeTracerFactory(myConfig). @dyladan @mayurkale22 I'd like to propose removing the singleton factory for this PR and revisit the idea in the future if a use case arises. How does that sound?

dyladan · 2019-10-30T14:54:58Z

imho we should be deviating from the spec as little as possible. If you believe the spec should be changed that is a separate issue. Maybe it could be written in a more language/implementation
agnostic manner? There is currently a working group to make the spec less language specific that might be interested in this.

Maybe the spec could, instead of specifying a static tracer factory with a delegate, say something along the lines of "support multiple tracer factory configurations for runtimes with multiple applications"

edit: I assume by "static" you are referring to the singleton pattern?

bg451 · 2019-10-30T15:02:25Z

Yeah meant singleton, still drinking my morning coffee :)

I don't think we'd be deviating much if at all from the specification as it doesn't call for a singleton. It does mention having a global tracer factory registry, but that is what initGlobalTracerFactory and getTracerFactory/getTracer covers.

dyladan · 2019-10-30T15:12:21Z

Yea I'm just looking over it now. It looks like it allows for the creation of multiple tracer factories, and the delegate uses the wording "may" which to me just suggests an implementation rather than requiring it.

austinlparker · 2019-10-30T15:53:02Z

FWIW I think it's worth remembering that a primary goal of the spec is, to quote,

This document defines common principles that will help designers create language libraries that are easy to use, are uniform across all supported languages, yet allow enough flexibility for language-specific expressiveness.

(from https://github.com/open-telemetry/opentelemetry-specification/blob/a131ae1ccabee8cc8f3b8cf109b81be378006fea/specification/library-guidelines.md)

I'm not quite sure what the most 'javascript-y' way to accomplish the named tracer factory is in this library, but I'd hew towards doing what feels natural for this language.

…try-node into bg/add_tracer_factory_1

…that

bg451 · 2019-10-31T18:52:10Z

@mayurkale22 @dyladan @OlivierAlbertini I've updated this to not use the singleton and did a bit of refactoring, ptal!

dyladan · 2019-10-31T22:08:13Z

packages/opentelemetry-tracing/src/AbstractBasicTracerFactory.ts

+import { BasicTracer } from './BasicTracer';
+import { SpanProcessor } from './SpanProcessor';
+
+export abstract class AbstractBasicTracerFactory


Why is the abstract factory specific to basic tracer? Why not just AbstractTracerFactory and _newTracer would return types.Tracer?

BasicTracerFactory and NodeTracerFactory would inherit from it as NodeTracer and BasicTracer both implement types.Tracer

It's due to this line specifically, since span processors is not an API level concept. https://github.com/open-telemetry/opentelemetry-js/pull/420/files#diff-7de0ceee2be4a0e5c9c8ba090fcc1ce4R28

But the base class they inherit from should be more general. BasicTracer and WebTracer are both just specializations of types.Tracer. The abstract base class is just an abstract tracer factory which all other tracer factories inherit from. Those more specific tracer factories would then have more specific typings.

class AbstractTracerFactory { protected abstract _newTracer(): types.Tracer; } class BasicTracerFactory { protected _newTracer(): BasicTracer { return new BasicTracer(); } } class WebTracerFactory { protected _newTracer(): WebTracer { return new WebTracer(); } }

Cool, updated the abstract factory to be more general.

dyladan · 2019-10-31T22:08:31Z

packages/opentelemetry-tracing/src/AbstractBasicTracerFactory.ts

+    return tracer;
+  }
+
+  protected abstract _newTracer(): BasicTracer;


Why is this BasicTracer and not types.Tracer?

dyladan · 2019-10-31T22:09:17Z

packages/opentelemetry-node/src/NodeTracerFactory.ts

-export class NodeTracerFactory implements types.TracerFactory {
-  private readonly _tracers: Map<string, NodeTracer> = new Map();
-  private _spanProcessors: SpanProcessor[] = [];
+export class NodeTracerFactory extends AbstractBasicTracerFactory {


It seems weird to me the NodeTracerFactory extends AbstractBasicTracerFactory when NodeTracer and BasicTracer are different

dyladan · 2019-10-31T22:10:00Z

packages/opentelemetry-web/src/WebTracerFactory.ts

+  BasicTracerConfig,
+} from '@opentelemetry/tracing';
+
+export class WebTracerFactory extends AbstractBasicTracerFactory {


Same here, I think it should just inherit from a more general AbstractTracerFactory

dyladan · 2019-10-31T22:10:23Z

packages/opentelemetry-web/src/WebTracerFactory.ts

+    this._config = config;
+  }
+
+  protected _newTracer(): BasicTracer {


Why is this BasicTracer return type not WebTracer?

obecny

I have added few minor comments, typos or just upvoted for the existing issue, there are some places which are missing jsdoc too. I think the example for examples/tracer-web is not updated.

obecny · 2019-11-05T21:30:43Z

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

+    this._tracerFactory = tracerFactory || null;
+    this._fallbackTracerFactory =
+      fallbackTracerFactory || new NoopTracerFactory();
+    this._currentTracerFactory =


should we have here this.start(); instead?

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

packages/opentelemetry-tracing/README.md

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

packages/opentelemetry-web/src/WebTracerFactory.ts

mayurkale22 · 2019-11-07T19:31:33Z

packages/opentelemetry-node/src/index.ts

@@ -15,3 +15,4 @@
 */

 export * from './NodeTracer';


Do you think we should remove this now? As NodeTracerFactory is responsible for tracer creation and all.

dyladan · 2019-11-08T15:39:21Z

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

+   * @param name identifies the instrumentation library
+   * @param [version] is the semantic version of the library.
+   */
+  getTracer(name: string, version?: string): types.Tracer {


should return type T

dyladan · 2019-11-08T15:40:35Z

packages/opentelemetry-tracing/src/BasicTracerFactory.ts

+    }
+  }
+
+  getTracer(name: string = '', version?: string): BasicTracer {


Is there a reason to override the default getTracer implementation?

dyladan · 2019-11-08T15:41:05Z

packages/opentelemetry-tracing/src/BasicTracerFactory.ts

+   * addSpanProcessor adds a {@link SpanProcessor} to the factory, applying it
+   * to all new and existing tracers.
+   */
+  addSpanProcessor(spanProcessor: SpanProcessor): void {


Why is this not on AbstractTracerFactory?

API vs SDK. Again, addSpanProcessor is a sdk concept and doesn't exist at the API level. If the AbstractTracerFactory is going to be in core and reusable by other implementations, it's should be at the level of the API.

dyladan · 2019-11-08T15:41:33Z

packages/opentelemetry-web/src/WebTracerFactory.ts

+/**
+ * WebTracerFactory produces named tracers.
+ */
+export class WebTracerFactory extends BasicTracerFactory {


Why does this inherit BasicTracerFactory rather than AbstractTracerFactory<WebTracer>>?

I don't see the reason why we would reimplement getTracer to do the addSpanProcessor loops, the only thing that's different is _newTracer. That being said I don't really have a strong opinion here, can go ahead and make the change

dyladan · 2019-11-08T15:42:34Z

packages/opentelemetry-node/src/NodeTracerFactory.ts

+import { NodeTracerConfig } from './config';
+import { BasicTracerFactory } from '@opentelemetry/tracing';
+
+export class NodeTracerFactory extends BasicTracerFactory {


Why does this inherit BasicTracerFactory rather than AbstractTracerFactory<NodeTracer>>?

I don't see the reason why we would reimplement getTracer to do the addSpanProcessor loops, the only thing that's different is _newTracer. That being said I don't really have a strong opinion here, can go ahead and make the change

dyladan · 2019-11-08T15:44:47Z

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

+import { NoopTracer } from './NoopTracer';
+
+export class NoopTracerFactory implements types.TracerFactory {
+  private readonly _tracer: types.Tracer;


Why not omit constructor entirely and do:

private readonly _tracer = new NoopTracer();

dyladan · 2020-01-22T16:47:31Z

Closing because it is not longer relevant

Brandon Gonzalez added 7 commits October 10, 2019 11:49

initial commit

09835be

ckpt

7f9d1bb

fix rebase

0f2f443

ftb

bbe8767

update examples

5b01c76

Adds test for noop factory

b3e5508

ftb

4d30e58

bg451 marked this pull request as ready for review October 10, 2019 21:54

bg451 requested review from danielkhan, iredelmeier, mayurkale22, OlivierAlbertini, rochdev and vmarchaud as code owners October 10, 2019 21:54

Merge branch 'master' into bg/add_tracer_factory_1

191eb4a

mayurkale22 reviewed Oct 11, 2019

View reviewed changes

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

mayurkale22 reviewed Oct 11, 2019

View reviewed changes

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

mayurkale22 reviewed Oct 11, 2019

View reviewed changes

packages/opentelemetry-tracing/src/BasicTracerFactory.ts Outdated Show resolved Hide resolved

bg451 and others added 2 commits October 14, 2019 13:55

Update packages/opentelemetry-types/src/trace/TracerFactory.ts

2930934

Co-Authored-By: Mayur Kale <[email protected]>

Update packages/opentelemetry-tracing/src/BasicTracerFactory.ts

527f0be

Co-Authored-By: Mayur Kale <[email protected]>

mayurkale22 mentioned this pull request Oct 15, 2019

feat(trace): named tracer factory prototype 2 #434

Closed

Brandon Gonzalez added 4 commits October 17, 2019 18:10

make factories static, add addSpanProcess to them

d8524db

Merge branch 'bg/add_tracer_factory_1' of github.com:bg451/openteleme…

868a88e

…try-node into bg/add_tracer_factory_1

readd instance

1e7f7f8

ftb

0324f3e

OlivierAlbertini reviewed Oct 18, 2019

View reviewed changes

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

packages/opentelemetry-node/src/NodeTracerFactory.ts Outdated Show resolved Hide resolved

dyladan reviewed Oct 24, 2019

View reviewed changes

Update packages/opentelemetry-tracing/src/BasicTracerFactory.ts

8695501

Co-Authored-By: dyladan <[email protected]>

bg451 requested a review from markwolff as a code owner October 29, 2019 21:01

Brandon Gonzalez and others added 4 commits October 30, 2019 16:31

remove singleton instances

b74e44a

Merge branch 'bg/add_tracer_factory_1' of github.com:bg451/openteleme…

8beea48

…try-node into bg/add_tracer_factory_1

refactor: Adds AbstractBasicTracerFactory, all factories extend from …

05abd69

…that

Merge branch 'master' into bg/add_tracer_factory_1

9150295

dyladan reviewed Oct 31, 2019

View reviewed changes

obecny reviewed Nov 5, 2019

View reviewed changes

bg451 and others added 5 commits November 6, 2019 11:30

Merge branch 'master' into bg/add_tracer_factory_1

4528fab

respond to comments

af4e624

respond to comments

6c1e42d

lint

8e9428a

changes abstract factory to use generic tracer type

27f16b1

bg451 requested a review from mayurkale22 November 7, 2019 19:01

mayurkale22 reviewed Nov 7, 2019

View reviewed changes

dyladan reviewed Nov 8, 2019

View reviewed changes

mayurkale22 added the feature-request label Nov 12, 2019

mayurkale22 added the Awaiting reviewer feedback label Nov 27, 2019

dyladan mentioned this pull request Dec 2, 2019

Named Tracers / Tracer Registry #582

Merged

dyladan closed this Jan 22, 2020

mayurkale22 self-assigned this Sep 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(trace): adds named tracer factory #420

feature(trace): adds named tracer factory #420

bg451 commented Oct 10, 2019 •

edited

Loading

dyladan left a comment

dyladan Oct 24, 2019

mayurkale22 Nov 7, 2019

dyladan Oct 24, 2019

dyladan Oct 24, 2019

bg451 Oct 30, 2019

dyladan commented Oct 25, 2019

bg451 commented Oct 30, 2019 •

edited

Loading

dyladan commented Oct 30, 2019 •

edited

Loading

bg451 commented Oct 30, 2019

dyladan commented Oct 30, 2019

austinlparker commented Oct 30, 2019

bg451 commented Oct 31, 2019

dyladan Oct 31, 2019

bg451 Nov 2, 2019

dyladan Nov 6, 2019

bg451 Nov 7, 2019

dyladan Oct 31, 2019

dyladan Oct 31, 2019

dyladan Oct 31, 2019

dyladan Oct 31, 2019

obecny left a comment

obecny Nov 5, 2019

mayurkale22 Nov 7, 2019

dyladan Nov 8, 2019

dyladan Nov 8, 2019

dyladan Nov 8, 2019

bg451 Nov 11, 2019 •

edited

Loading

dyladan Nov 8, 2019

bg451 Nov 11, 2019

dyladan Nov 8, 2019

bg451 Nov 11, 2019

dyladan Nov 8, 2019

dyladan commented Jan 22, 2020

feature(trace): adds named tracer factory #420

feature(trace): adds named tracer factory #420

Conversation

bg451 commented Oct 10, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

dyladan 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

dyladan commented Oct 25, 2019

bg451 commented Oct 30, 2019 • edited Loading

dyladan commented Oct 30, 2019 • edited Loading

bg451 commented Oct 30, 2019

dyladan commented Oct 30, 2019

austinlparker commented Oct 30, 2019

bg451 commented Oct 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny 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

bg451 Nov 11, 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

dyladan commented Jan 22, 2020

bg451 commented Oct 10, 2019 •

edited

Loading

bg451 commented Oct 30, 2019 •

edited

Loading

dyladan commented Oct 30, 2019 •

edited

Loading

bg451 Nov 11, 2019 •

edited

Loading