Skip to content
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

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

Closed
wants to merge 2 commits into from

Conversation

mayurkale22
Copy link
Member

Which problem is this PR solving?

/cc @bg451

}

// @todo
// addSpanProcessor(spanProcessor: SpanProcessor): void { }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it that this would be applied to all tracers in the set and to all new tracers created afterwards?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, correct.

@codecov-io
Copy link

codecov-io commented Oct 15, 2019

Codecov Report

Merging #434 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
- Coverage   95.88%   95.87%   -0.02%     
==========================================
  Files         108      108              
  Lines        5347     5333      -14     
  Branches      433      435       +2     
==========================================
- Hits         5127     5113      -14     
  Misses        220      220
Impacted Files Coverage Δ
...opentelemetry-base/test/resources/resource.test.ts 100% <0%> (ø) ⬆️

@danielkhan
Copy link
Contributor

Are there good reasons for the redundancy between BasicTracerFactory and NodeTracerFactory? This looks like a case where inheritance might make sense but maybe I miss something here.

@OlivierAlbertini
Copy link
Member

LGTM

@dyladan
Copy link
Member

dyladan commented Oct 25, 2019

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();
  }
}

@mayurkale22
Copy link
Member Author

Hey @dyladan original PR is #420, feel free to add comments on that. @bg451 has already merged changes from this to #420. Please ignore this PR, will close it soon.

@mayurkale22
Copy link
Member Author

Closing this in favor of #420

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants