Exporting TracerBase as a separate @opencensus/nodejs-base package #495

hekike · 2019-05-03T16:57:55Z

As @opencensus/nodejs exports a Tracing instance as default, which also initializes CLS I would like to propose a new @opencensus/nodejs-base package to avoid breaking change for existing customers. The existing @opencensus/nodejs become a thin layer on the new @opencensus/nodejs-base after this change.

The alternative solution would be to only export Tracing and TracingBase classes and let the user initialize them. It would also require to start CLS only when Tracing is initialized instead of the current global instance. This would be a breaking change.

This PR is related to #484

draffensperger

I like this idea of splitting out the complexity of the CLS based tracer and giving the option of fully manual spans.

I left a few comments. I think we should wait for @mayurkale22 to come back Monday to get this approved and merged.

draffensperger · 2019-05-03T17:02:40Z

packages/opencensus-nodejs-base/package.json

+  },
+  "dependencies": {
+    "@opencensus/core": "^0.0.11",
+    "@opencensus/instrumentation-all": "^0.0.11",


Do you still need this dependency if no plugins are pulled in by default?

draffensperger · 2019-05-03T17:03:09Z

packages/opencensus-nodejs-base/src/trace/instrumentation/plugin-loader.ts

@@ -33,7 +33,7 @@ enum HookState {
 */
 export class PluginLoader {
  /** The tracer */
-  private tracer: Tracer;
+  private tracer: Tracer|TracerBase;


Can this just be TracerBase since Tracer inherits from it? Same below.

draffensperger · 2019-05-03T17:04:42Z

packages/opencensus-nodejs/README.md

-    rootSpan.end();
-});
-```
+For manual insturmentation see the `@opencensus/nodejs-base` package.


What if someone wants both automatic and manual instrumentation? I would think they would be able to do it by using the @opencensus/nodejs package as well? Maybe we should still have the doc above, but mention that if you only want manual instrumentation to see the @opencensus/nodejs-base package?

That's a very good point, I'll revert it and add the extra context.

…oads

mayurkale22

Please fix the build. Also update CHANGELOG.md

mayurkale22 · 2019-05-07T20:51:49Z

packages/opencensus-nodejs-base/AUTHORS

+# of contributors, see the revision history in source control.
+
+Google LLC
+CESAR Team (www.cesar.org.br)


I think we should remove "CESAR Team" and if you want add "Netflix".

mayurkale22 · 2019-05-07T20:55:17Z

packages/opencensus-nodejs-base/README.md

+
+For automatic insturmentation see the `@opencensus/nodejs` package.
+
+### Manually Instrument an Application


Nit: s/Instrument/Instrumenting to maintain consistency.

mayurkale22 · 2019-05-07T20:56:03Z

packages/opencensus-nodejs-base/codecov.yml

@@ -0,0 +1,2 @@
+fixes:
+  - "::packages/opencensus-nodejs/"


s/opencensus-nodejs/opencensus-nodejs-base?

mayurkale22 · 2019-05-07T21:00:04Z

packages/opencensus-nodejs-base/src/trace/tracing-base.ts

@@ -0,0 +1,132 @@
+/**
+ * Copyright 2018, OpenCensus Authors


Nit: s/2018/2019

mayurkale22 · 2019-05-07T21:32:29Z

packages/opencensus-nodejs/README.md

@@ -61,6 +63,8 @@ tracing.tracer.startRootSpan(rootSpanOptions, (rootSpan) => {
 });
 ```

+For manual only instrumentation see the `@opencensus/nodejs-base` package.


You may want to add link for the @opencensus/nodejs-base package.

mayurkale22 · 2019-05-07T21:34:28Z

packages/opencensus-nodejs/package.json

-    "extend": "^3.0.2",
-    "require-in-the-middle": "^4.0.0"
+    "@opencensus/nodejs-base": "^0.0.11",
+    "@opencensus/instrumentation-all": "^0.0.11"


Nit: Move @opencensus/instrumentation-all above @opencensus/nodejs-base to keep the ordering intact.

hekike · 2019-05-07T23:23:00Z

@mayurkale22 thanks for the review. I'm trying to fix the build for days now and I couldn't figure out what's happening. It was passing on my machine earlier, now with a clean copy, I can least reproduce it. I feel it's more a mono repo thing and I'm missing something. Any chance you have a hint?

hekike · 2019-05-08T17:35:52Z

@mayurkale22 I could figure out, both CI and your comments are fixed now.

codecov-io · 2019-05-08T17:37:04Z

Codecov Report

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

@@            Coverage Diff            @@
##           master     #495     +/-   ##
=========================================
- Coverage   94.97%   94.68%   -0.3%     
=========================================
  Files         145      145             
  Lines       10091    10231    +140     
  Branches      882      870     -12     
=========================================
+ Hits         9584     9687    +103     
- Misses        507      544     +37

Impacted Files	Coverage Δ
src/zpages-frontend/page-handlers/templates-dir.ts	`80% <0%> (-20%)`	⬇️
test/test-http2.ts	`79.47% <0%> (-17.35%)`	⬇️
src/trace/propagation/noop-propagation.ts	`40% <0%> (-10%)`	⬇️
src/common/noop-logger.ts	`11.11% <0%> (-5.56%)`	⬇️
test/test-instana.ts	`85.18% <0%> (-3.71%)`	⬇️
src/detect-resource.ts	`90.9% <0%> (-1.95%)`	⬇️
test/test-detect-resource.ts	`99.13% <0%> (-0.38%)`	⬇️
src/internal/string-utils.ts	`100% <0%> (ø)`	⬆️
test/test-tag-map.ts	`100% <0%> (ø)`	⬆️
src/binary-format.ts	`100% <0%> (ø)`	⬆️
... and 32 more

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 96a6ab7...f0f27de. Read the comment docs.

mayurkale22

Overall looks good to me, added a few comments.

mayurkale22 · 2019-05-08T17:54:38Z

package.json

@@ -6,6 +6,7 @@
  "types": "build/src/index.d.ts",
  "repository": "census-instrumentation/opencensus-node",
  "scripts": {
+    "fix": "lerna run fix",


Ideally, I would prefer to do this kind of (unrelated) changes in separate PR, just to keep PR clean and small. :)

mayurkale22 · 2019-05-08T18:06:10Z

packages/opencensus-nodejs-base/test/test-tracing.ts

+  describe('start()', () => {
+    let aTracingBase: core.Tracing;
+    const tracing = new TracingBase();
+    // tslint:disable:no-any


Could you please remove this TSLint rule flag?

mayurkale22 · 2019-05-08T18:11:06Z

packages/opencensus-nodejs/src/trace/tracing.ts

@@ -1,5 +1,5 @@
 /**
- * Copyright 2018, OpenCensus Authors
+ * Copyright 2019, OpenCensus Authors


Usually, we don't update the copyright header when we touch the file.

hekike · 2019-05-08T18:21:49Z

@mayurkale22 thanks, fixed and separate PR opened for lerna fix #508

mayurkale22 · 2019-05-08T20:06:55Z

Just wondering: What do you think about renaming this package to either @opencensus/basictracer-nodejs or @opencensus/basictracer or @opencensus/basetracer-nodejs or something similar? For me, existing @opencensus/nodejs name does not make good sense, but it is hard to change that at this moment.

hekike · 2019-05-08T20:32:47Z

I also don't like the current name, but wouldn't it be worst to have two different naming concepts in the same project? I would keep the current naming convention in OpenCensus and fix it in OpenTelemetry.

mayurkale22 · 2019-05-08T20:57:12Z

Ok makes sense.

hekike · 2019-05-08T21:15:12Z

For OpenTelemetry, I don't even think we need nodejs in the naming. npm is for JS packages.
it could be just:

@opencensus/basictracer
@opencensus/tracer

Or even more building block philosophy:

@opencensus/tracer (basic tracer)
@opencensus/context-propagation (cls)
@opencensus/automatic (starting tracer with cls: @opencensus/tracer wrapped with @opencensus/context-propagation)

mayurkale22 · 2019-05-08T22:35:46Z

I will merge this later today, if no other comments from either @draffensperger or @justindsmith

hekike · 2019-05-09T16:54:31Z

@mayurkale22 can we also cut a release when it's merged?

mayurkale22 · 2019-05-09T17:14:12Z

@mayurkale22 can we also cut a release when it's merged?

Sure, I would like to include #503 and #510 (if possible #454). FYI (Release Milestone) : https://github.com/census-instrumentation/opencensus-node/milestone/3. I will cut a release later tomorrow. Is that ok?

hekike · 2019-05-09T17:16:52Z

Perfect, thanks!

mayurkale22 · 2019-05-13T16:42:10Z

@mayurkale22 can we also cut a release when it's merged?

The release (0.0.12) is completed for @opencensus/* packages. PTAL.

hekike requested review from draffensperger, justindsmith and mayurkale22 as code owners May 3, 2019 16:57

googlebot added the cla: yes label May 3, 2019

draffensperger reviewed May 3, 2019

View reviewed changes

Peter Marton added 4 commits May 6, 2019 13:48

feat(opencensus-node): export base tracing

afef52b

feat(nodejs-base): export TracerBase without CLS and default plugin l…

be21824

…oads

refactor(nodejs-base): address PR comments

3c8c180

refactor(nodejs-base): address PR comments

5665abe

hekike force-pushed the feat/export-base-tracing branch from 275eca0 to 5665abe Compare May 6, 2019 20:55

chore(nodejs): remove unused deps

d3e37de

mayurkale22 reviewed May 7, 2019

View reviewed changes

Peter Marton added 2 commits May 8, 2019 10:19

test(nodejs-base): plugin laoder test refactors

2ef770f

test(nodejs-base): add back test/instrumentation folder

28269c5

mayurkale22 approved these changes May 8, 2019

View reviewed changes

fix(nodejs-base): PR comments

f0f27de

mayurkale22 added this to the Release 0.0.12 milestone May 9, 2019

mayurkale22 merged commit 53dab13 into census-instrumentation:master May 9, 2019

mayurkale22 mentioned this pull request May 30, 2019

Proposal: Package Structure open-telemetry/opentelemetry-js#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exporting TracerBase as a separate @opencensus/nodejs-base package #495

Exporting TracerBase as a separate @opencensus/nodejs-base package #495

hekike commented May 3, 2019 •

edited

Loading

draffensperger left a comment

draffensperger May 3, 2019

draffensperger May 3, 2019

draffensperger May 3, 2019

hekike May 3, 2019

mayurkale22 left a comment

mayurkale22 May 7, 2019

mayurkale22 May 7, 2019

mayurkale22 May 7, 2019

mayurkale22 May 7, 2019

mayurkale22 May 7, 2019

mayurkale22 May 7, 2019

hekike commented May 7, 2019 •

edited

Loading

hekike commented May 8, 2019

codecov-io commented May 8, 2019 •

edited

Loading

mayurkale22 left a comment

mayurkale22 May 8, 2019

mayurkale22 May 8, 2019

mayurkale22 May 8, 2019

hekike commented May 8, 2019

mayurkale22 commented May 8, 2019

hekike commented May 8, 2019

mayurkale22 commented May 8, 2019

hekike commented May 8, 2019 •

edited

Loading

mayurkale22 commented May 8, 2019

hekike commented May 9, 2019

mayurkale22 commented May 9, 2019

hekike commented May 9, 2019

mayurkale22 commented May 13, 2019


		For automatic insturmentation see the `@opencensus/nodejs` package.

		### Manually Instrument an Application

Exporting TracerBase as a separate @opencensus/nodejs-base package #495

Exporting TracerBase as a separate @opencensus/nodejs-base package #495

Conversation

hekike commented May 3, 2019 • edited Loading

draffensperger 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

mayurkale22 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

Choose a reason for hiding this comment

hekike commented May 7, 2019 • edited Loading

hekike commented May 8, 2019

codecov-io commented May 8, 2019 • edited Loading

Codecov Report

mayurkale22 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

hekike commented May 8, 2019

mayurkale22 commented May 8, 2019

hekike commented May 8, 2019

mayurkale22 commented May 8, 2019

hekike commented May 8, 2019 • edited Loading

mayurkale22 commented May 8, 2019

hekike commented May 9, 2019

mayurkale22 commented May 9, 2019

hekike commented May 9, 2019

mayurkale22 commented May 13, 2019

hekike commented May 3, 2019 •

edited

Loading

hekike commented May 7, 2019 •

edited

Loading

codecov-io commented May 8, 2019 •

edited

Loading

hekike commented May 8, 2019 •

edited

Loading