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

feature(trace): adds named tracer factory #420

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor: Adds AbstractBasicTracerFactory, all factories extend from …
…that
  • Loading branch information
Brandon Gonzalez committed Oct 31, 2019
commit 05abd696898af26bfef63d3ba5b4f1a7dcab1150
30 changes: 8 additions & 22 deletions packages/opentelemetry-node/src/NodeTracerFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,36 +14,22 @@
* limitations under the License.
*/

import * as types from '@opentelemetry/types';
import { NodeTracer } from './NodeTracer';
import { NodeTracerConfig } from './config';
import { SpanProcessor } from '@opentelemetry/tracing';
import {
BasicTracer,
AbstractBasicTracerFactory,
} from '@opentelemetry/tracing';

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

Choose a reason for hiding this comment

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

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

private readonly _config?: NodeTracerConfig;

constructor(config?: NodeTracerConfig) {
super();
this._config = config;
}

addSpanProcessor(processor: SpanProcessor): void {
this._spanProcessors.push(processor);
for (const tracer of this._tracers.values()) {
tracer.addSpanProcessor(processor);
}
}

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

const tracer = new NodeTracer(this._config);
for (const processor of this._spanProcessors) {
tracer.addSpanProcessor(processor);
}
this._tracers.set(key, tracer);
return tracer;
protected _newTracer(): BasicTracer {
return new NodeTracer(this._config);
}
}
46 changes: 46 additions & 0 deletions packages/opentelemetry-tracing/src/AbstractBasicTracerFactory.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*!
* Copyright 2019, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as types from '@opentelemetry/types';
import { BasicTracer } from './BasicTracer';
import { SpanProcessor } from './SpanProcessor';

export abstract class AbstractBasicTracerFactory
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, updated the abstract factory to be more general.

implements types.TracerFactory {
protected readonly _tracers: Map<string, BasicTracer> = new Map();
protected _spanProcessors: SpanProcessor[] = [];

addSpanProcessor(spanProcessor: SpanProcessor): void {
this._spanProcessors.push(spanProcessor);
for (const tracer of this._tracers.values()) {
tracer.addSpanProcessor(spanProcessor);
}
}

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

const tracer = this._newTracer();
for (const processor of this._spanProcessors) {
tracer.addSpanProcessor(processor);
}
this._tracers.set(key, tracer);
return tracer;
}

protected abstract _newTracer(): BasicTracer;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this BasicTracer and not types.Tracer?

}
27 changes: 5 additions & 22 deletions packages/opentelemetry-tracing/src/BasicTracerFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,36 +14,19 @@
* limitations under the License.
*/

import * as types from '@opentelemetry/types';
import { BasicTracerConfig } from './types';
import { BasicTracer } from './BasicTracer';
import { SpanProcessor } from './SpanProcessor';
import { AbstractBasicTracerFactory } from './AbstractBasicTracerFactory';

export class BasicTracerFactory implements types.TracerFactory {
private readonly _tracers: Map<string, BasicTracer> = new Map();
private _spanProcessors: SpanProcessor[] = [];
export class BasicTracerFactory extends AbstractBasicTracerFactory {
private _config?: BasicTracerConfig;

constructor(config?: BasicTracerConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

super();
this._config = config;
}

addSpanProcessor(spanProcessor: SpanProcessor): void {
this._spanProcessors.push(spanProcessor);
for (const tracer of this._tracers.values()) {
tracer.addSpanProcessor(spanProcessor);
}
}

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

const tracer = new BasicTracer(this._config);
for (const processor of this._spanProcessors) {
tracer.addSpanProcessor(processor);
}
this._tracers.set(key, tracer);
return tracer;
_newTracer(): BasicTracer {
return new BasicTracer(this._config);
}
}
1 change: 1 addition & 0 deletions packages/opentelemetry-tracing/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,4 @@ export * from './export/SpanExporter';
export * from './Span';
export * from './SpanProcessor';
export * from './types';
export * from './AbstractBasicTracerFactory';
35 changes: 35 additions & 0 deletions packages/opentelemetry-web/src/WebTracerFactory.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*!
* Copyright 2019, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { WebTracer } from './WebTracer';
import {
BasicTracer,
AbstractBasicTracerFactory,
BasicTracerConfig,
} from '@opentelemetry/tracing';

export class WebTracerFactory extends AbstractBasicTracerFactory {
Copy link
Member

Choose a reason for hiding this comment

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

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

private readonly _config?: BasicTracerConfig;
bg451 marked this conversation as resolved.
Show resolved Hide resolved

constructor(config?: BasicTracerConfig) {
super();
this._config = config;
}

protected _newTracer(): BasicTracer {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this BasicTracer return type not WebTracer?

return new WebTracer(this._config);
}
}