Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Refactor Tracer to check presence of Zone global variable #103

Merged

Conversation

crdgonzalezca
Copy link
Contributor

This checks the presence of Zone in Tracer as there might be some case where zone.js library is not imported.
Also, modify Tracer to be Singleton as there will be only one instance.

@codecov-io
Copy link

codecov-io commented Jun 24, 2019

Codecov Report

Merging #103 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   96.23%   96.29%   +0.05%     
==========================================
  Files          26       26              
  Lines         637      647      +10     
  Branches       80       83       +3     
==========================================
+ Hits          613      623      +10     
  Misses         24       24
Impacted Files Coverage Δ
src/trace/model/tracer.ts 97.22% <0%> (+1.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 484fbe7...abfc660. Read the comment docs.

private static singletonInstance: Tracer;

/** Gets the tracer instance. */
static get instance(): Tracer {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the motivation for adding this instance getter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that opencensus node does it and I think is more understandable that Tracer is a Singleton class.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, makes sense.

packages/opencensus-web-core/src/trace/model/tracer.ts Outdated Show resolved Hide resolved
packages/opencensus-web-core/src/trace/model/tracer.ts Outdated Show resolved Hide resolved
packages/opencensus-web-core/src/trace/model/tracer.ts Outdated Show resolved Hide resolved
@@ -23,7 +23,7 @@ export const NOOP_EXPORTER = new NoopExporter();
/** Main interface for tracing. */
export class Tracing implements webTypes.Tracing {
/** Object responsible for managing a trace. */
readonly tracer = new Tracer();
readonly tracer = Tracer.instance;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this similar to how OpenCensus Node does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I saw they do it like this, and it is more understandable that this class is Singleton.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

packages/opencensus-web-core/test/test-tracer.ts Outdated Show resolved Hide resolved
@draffensperger draffensperger merged commit a0d573f into census-instrumentation:master Jun 24, 2019
@crdgonzalezca crdgonzalezca deleted the refactoring branch July 24, 2019 15:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants