feat: add opencensus-core package by fabiogomessilva · Pull Request #2 · census-instrumentation/opencensus-node

fabiogomessilva · 2018-04-25T14:25:08Z

This PR contains opencensus nodejs core package.

The core package includes types and classes for opencensus domain model, exporters, and instrumentations: Span, RootSpan, Tracer, Sampler, Exporters, and Plugins (for instrumentation). Including also the Tracing interface and a default implementation for a logger and a console exporter. Unit tests included.

Opencensus-node packages are managed by Lerna.

…ving class files Span.ts, Trace.ts and Tracer.ts to trace/model

… interface, add TraceContext interface

…ypes

…odel to span-base

googlebot · 2018-04-25T14:25:43Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

kjin

If you feel that any of these comments should be done in a separate PR, please make a note of that in the form of a TODO comment (so that we can keep track).

Feel free to start a discussion on any comment that you might be confused on or disagree with. If it's not worth creating a full reply, you can comment "done" or use the 👍 reaction to help keep track of which comments have been addressed.

kjin · 2018-04-26T00:26:18Z

+  "author": "Google Inc.",
+  "license": "Apache-2.0",
+  "engines": {
+    "node": ">=6.00"


Nit: Remove the extra 0 here.

kjin · 2018-04-26T00:28:10Z


-OpenCensus is a toolkit for collecting application performance and behavior data. It currently 
-includes 3 apis: stats, tracing and tags.
+OpenCensus Node.js is an implementation of OpenCensus, a toolkit for collecting application performance and behavior monitoring data. Right now OpenCensus for Node.js supports custom tracing and automatic tracing for http and mongodb.


A few nits:

We should be consistent between OpenCensus Node.js and OpenCensus for Node.js (I prefer the latter)

Right now -> Currently

Codify http and mongodb to suggest that these are modules

For this PR I've simplified even more the README.md text. Even though I didn't add an explicit TODO, obviously it is a TODO for a next PR. Ok?

kjin · 2018-04-26T00:28:33Z

+- v6.10.0 (for console exporter only)
+- v9.8.0 (for Stackdriver and Zipkin exporters)
+
+___


Nit: Remove underlines

kjin · 2018-04-26T00:29:02Z

+  "version": "0.0.1",
+  "description": "OpenCensus is a toolkit for collecting application performance and behavior data.",
+  "main": "build/src/index.js",
+  "types": "build/src/index.d.js",


index.d.ts

kjin · 2018-04-26T00:29:26Z

@@ -0,0 +1 @@
+Google Inc.


Should this be OpenCensus Authors?

Others opencensus implementations are in the same state:
https://github.com/census-instrumentation/opencensus-java/blob/master/AUTHORS
https://github.com/census-instrumentation/opencensus-go/blob/master/AUTHORS

You should add CESAR to this list and/or individual contributors

kjin · 2018-04-26T01:58:47Z

+
+const tracer = new Tracer();
+
+describe('Span', () => {


Would it be possible to merge similar tests between RootSpan and Span? You can do so in a follow-up PR if needed.

TODO. We gonna evaluate that.

kjin · 2018-04-26T02:00:27Z

+  describe('get active()', () => {
+    let tracer: Tracer;
+    before(() => {
+      tracer = new Tracer();


Add an assertion here to ensure that it wasn't active beforehand.

kjin · 2018-04-26T02:00:50Z

+  });
+
+  /** Should return a started tracer instance */
+  describe('start()', () => {


This test seems nearly identical to get active().

Test case for get active() was removed.

kjin · 2018-04-26T02:03:25Z

+  });
+
+
+  /** Testing void functions */


What are these cases testing?

Test cases for tracer were updated.

kjin · 2018-04-26T02:03:56Z

+    "sourceMap": false
+  },
+  "include": [
+    "src/*.ts",


This needs to be "src/**/*.ts" or npm run fix will not work.

kjin · 2018-04-30T15:58:13Z

-  logger['logger'] = defaultLogger;
-  return defaultLogger;
-};
+const logger = (options?: types.LoggerOptions|string|number) => {


Is it necessary to allow options to be a number? LoggerOptions only accepts the logging level as a string.

logLevel is a number in Config interface. So, we decide to keep the logic to convert logLevel numbers to string in that class. I will add a TODO for reevaluation.

kjin · 2018-04-30T16:01:07Z


-logger();
+  const aLogger = new ConsoleLogger(opt);
+  logger['logger'] = aLogger;


If you call logger() multiple times, you get different loggers. If that is desired, I don't see why you need to store the logger on the ['logger'] property. If that is not desired, you should add something like if (logger['logger']) {...} to avoid re-creating the object each time.

If we always return the same logger instance, will not be possible to have loggers with different log levels at the same time, what can limit some test capability. I will add a TODO for reevaluation.

kjin · 2018-04-30T16:07:24Z

+   * @param name The function name.
+   * @param wrapper The wrapper.
+   */
+  protected wrap(nodule, name, wrapper) {


It's OK for instrumentations to depend on shimmer since npm will install a single module for all of them anyway, so there is no dependency size penalty. I would still prefer to keep the API as minimal as possible, especially functions like these that are little more than call-throughs to an underlying dependency module.

kjin · 2018-04-30T16:13:47Z

   * @param parentSpanId The parent span ID.
   */
-  startSpan(name?: string, type?: string, parentSpanId?: string): types.Span {
+  startChildSpan(name?: string, type?: string, parentSpanId?: string):


I believe parentSpanId doesn't need to be here? It's guaranteed to be this.spanId.

kjin · 2018-04-30T16:14:54Z


  /** Gives the TraceContext of the span. */
-  get traceContext(): types.TraceContext {
+  get spanContext(): types.SpanContext {


Why was the type/field changed to SpanContext? I thought TraceContext was fine. (If you decide to keep SpanContext, please change the code comment above.)

Actually, also looking at this more closely -- does the parentSpanId need to be there? It's not part of the SpanContext interface. Also, it would be great to have options here as well.

Why was the type/field changed to SpanContext?

It was changed to be consistent with opencensus that uses the term SpanContext:
https://github.com/census-instrumentation/opencensus-java/blob/master/api/src/main/java/io/opencensus/trace/SpanContext.java

Actually, also looking at this more closely -- does the parentSpanId need to be there?

There is no parentSpanId in opencensus SpanContext interface. As I understand the spanId should represent a parendSpanID when the context is propagated.

kjin · 2018-04-30T16:31:31Z

+/**
+ * This class represent a tracer.
+ */
+export class Tracer implements types.Tracer {


I like the idea to keep types depending on types and not on concrete implementations.

Generally, I agree. However it depends on the use case. In my mind there are two reasons for a dependent module to use a type definition -- either to extend it, or to instantiate/handle objects of that type. Interfaces like Exporter, Plugin, Logger, and so on are meant to extended by other modules, while Tracer falls in the latter category of being used by other modules, but not extended. That's why I believe that exporters, plugins, etc should be interfaces while Tracer should be a single class.

As we discussed in person, we should consider in the future whether there needs to be a specific interface for Tracer. Keep in mind that type definitions exported by this module don't contain any implementations whatsoever anyway, so other modules will always depend on types only and not implementations. The difference is whether the type definitions will say interface Tracer ... class Tracer or just class Tracer.

kjin · 2018-04-30T16:33:47Z

 import {Logger} from '../src/common/types';
-import {Buffer} from '../src/exporters/buffer';
 import {ConsoleExporter} from '../src/exporters/console-exporter';
+import {ExporterBuffer} from '../src/exporters/exporter-buffer';


Nit: Remove unused imports

kjin · 2018-04-30T16:36:14Z

+  describe('setBufferSize', () => {
+    it('should set BufferSize', () => {
+      const buffer = new ExporterBuffer(exporter, defaultBufferConfig);
+      const bufferResize = buffer.setBufferSize(DEFAULT_BUFFER_SIZE);


I would suggest using a value here other than DEFAULT_BUFFER_SIZE.

kjin · 2018-04-30T16:38:38Z

+      assert.strictEqual(sampler.description, 'always');
+      assert.ok(samplerShouldSampler);
+    });
+    it('should return a always sampler', () => {


Use different test case messages for these two tests (same for never sampler)

kjin · 2018-04-30T16:39:43Z

+    } as types.SpanContext;
+
+
+    it('should reate the new RootSpan with propagation', () => {


Nit: create

kjin

Looks mostly good to me -- I added a few final comments. Once you've addressed them we should be good to merge.

kjin · 2018-05-01T16:12:00Z

+    "nyc": "11.6.0"
+  },
+  "dependencies": {
+    "async_hooks": "^1.0.0",


It looks like it wasn't removed? Did I miss something?

kjin · 2018-05-01T16:33:21Z

+          }
+        }
+        const aRoot = new RootSpan(this, options);
+        const sampleDecisition: boolean = propagatedSample ?


sampleDecisition -> sampleDecision

kjin · 2018-05-01T16:37:38Z

@@ -0,0 +1,100 @@
+/**
+ * Copyright 2018 Google LLC. All Rights Reserved.


Change copyright here.

This reverts commit 67860ba.

The JaegerTraceExporterOptions accepts `tags` as a Tag array, but was processing those tags for the process information as if they were an object / Record instead of a Tag array. This was leading to the process key-value inside Jaeger to be the array index as key and the value as the key-value object. We now use the proper key-value information from the input to place into the process tags. There were two approaches we could have taken here: 1. To leave the input as a Tag array and process the input properly in the code or 2. To change the input to a key-value record / object (Record<string, TagValue), which would be handled properly by the existing code. We have chosen approach census-instrumentation#1 because a Jaeger process tag CAN support multiple tags with the same key name, which would not be supported in approach census-instrumentation#2. There is a chance that (because we were not previously exporting typescript types) users were passing in a Record format instead of Array, and thus this might be seen as a breaking change. A test has been added to validate the process information to ensure proper format. Also included are sticter typing for all thrift types, removing all `any` references and replacing with valid types.

The JaegerTraceExporterOptions accepts `tags` as a Tag array, but was processing those tags for the process information as if they were an object / Record instead of a Tag array. This was leading to the process key-value inside Jaeger to be the array index as key and the value as the key-value object. We now use the proper key-value information from the input to place into the process tags. There were two approaches we could have taken here: 1. To leave the input as a Tag array and process the input properly in the code or 2. To change the input to a key-value record / object (Record<string, TagValue), which would be handled properly by the existing code. We have chosen approach #1 because a Jaeger process tag CAN support multiple tags with the same key name, which would not be supported in approach #2. There is a chance that (because we were not previously exporting typescript types) users were passing in a Record format instead of Array, and thus this might be seen as a breaking change. A test has been added to validate the process information to ensure proper format. Also included are sticter typing for all thrift types, removing all `any` references and replacing with valid types.

…on#93) The JaegerTraceExporterOptions accepts `tags` as a Tag array, but was processing those tags for the process information as if they were an object / Record instead of a Tag array. This was leading to the process key-value inside Jaeger to be the array index as key and the value as the key-value object. We now use the proper key-value information from the input to place into the process tags. There were two approaches we could have taken here: 1. To leave the input as a Tag array and process the input properly in the code or 2. To change the input to a key-value record / object (Record<string, TagValue), which would be handled properly by the existing code. We have chosen approach census-instrumentation#1 because a Jaeger process tag CAN support multiple tags with the same key name, which would not be supported in approach census-instrumentation#2. There is a chance that (because we were not previously exporting typescript types) users were passing in a Record format instead of Array, and thus this might be seen as a breaking change. A test has been added to validate the process information to ensure proper format. Also included are sticter typing for all thrift types, removing all `any` references and replacing with valid types.

silva-fabio and others added 30 commits March 20, 2018 11:42

PoD(Proof Of Design) - v0

26c75a0

Added basic structure of tests

ce6d020

finished test-trace

f482e37

added new tests in test-trace

8c9102d

Add unit tests for span.ts

532819a

chord: merge commit

38aea52

finished test-tracemanager

891202e

chord: merge commit

3a1fd0d

Add initial Stackdriver exporter

5126f58

added test-tracemanager

a221c63

finished interfaces creation

5c853bf

Fix end trace

994b9bc

Add interfaces

7161ccf

Add license on the unit tests

50d266d

chord: merge commit

99a090b

chord: merge commit

bfed75d

Changing class name TraceManger to Tracer, creating class Tracing. Mo…

3cf908a

…ving class files Span.ts, Trace.ts and Tracer.ts to trace/model

Changing class name TraceManger to Tracer, creating class Tracing. Mo…

7af710f

…ving class files Span.ts, Trace.ts and Tracer.ts to trace/model

Refactoring test classes to the new clases names

bf33d9c

Send real data to Stackdriver and create registerExporter method

072bfc4

traceId logic update

a40f1d4

Fix code style.

3b86c76

chord: merge commit

0790f49

Make spans in a trace private

abad01f

Add start methods for Stackdriver and Zipkin

1cff931

chord: merge commit

7491376

added basic struct

50b920d

finished zipkin basic implementation

d6930df

chord: merge commit

172bf1b

Refactoring: rename class and file Trace to RootSpan, add TraceConfig…

de75000

… interface, add TraceContext interface

luanamartinssantos and others added 6 commits April 24, 2018 21:04

refactor: remove Impl classes suffixes and use module namespace for t…

d01e0f4

…ypes

refactor: update and standardize use of types module namespace

7dca176

style: update Copyright notice

e7036c8

refactor: rename class SpanBaseModel to SpanBase and file span-base-m…

72a97c2

…odel to span-base

doc: remove doc files unrelated to core

20fef5c

doc: update tracer jsdoc for wrap and wraEmitter

618b9d6

kjin self-requested a review April 25, 2018 17:21

kjin reviewed Apr 26, 2018

View reviewed changes

fabiogomessilva force-pushed the master branch 2 times, most recently from 1efd7e5 to 5ba700f Compare April 30, 2018 10:26

refactor: changes after PR review

b90d44c

fabiogomessilva force-pushed the master branch from 5ba700f to b90d44c Compare April 30, 2018 14:25

kjin reviewed Apr 30, 2018

View reviewed changes

refactor: rework according to review follow up

c8b2ffb

fabiogomessilva force-pushed the master branch from 4707152 to c8b2ffb Compare May 1, 2018 01:59

kjin approved these changes May 1, 2018

View reviewed changes

kjin reviewed May 1, 2018

View reviewed changes

refactor: fix some dependencies, file copyright and typos

8e83290

kjin approved these changes May 1, 2018

View reviewed changes

tags07 merged commit 67860ba into census-instrumentation:master May 1, 2018

tags07 added a commit that referenced this pull request May 1, 2018

Revert "feat: add opencensus-core package (#2)"

824aba9

This reverts commit 67860ba.

tags07 mentioned this pull request May 1, 2018

Revert "feat: add opencensus-core package " #3

Closed

kjin mentioned this pull request Jun 20, 2018

refactor: address opencensus-core TODOs #47

Merged

jdbennet2001 mentioned this pull request Jun 3, 2019

Fixes #566: set CurrentRootSpan whenever created span is RootSpan #570

Merged

		} as types.SpanContext;


		it('should reate the new RootSpan with propagation', () => {

		@@ -0,0 +1,100 @@
		/**
		* Copyright 2018 Google LLC. All Rights Reserved.

Conversation

fabiogomessilva commented Apr 25, 2018

Uh oh!

googlebot commented Apr 25, 2018

Uh oh!

kjin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabiogomessilva Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabiogomessilva Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabiogomessilva Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabiogomessilva Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjin Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

fabiogomessilva Apr 27, 2018 •

edited

Loading

fabiogomessilva Apr 27, 2018 •

edited

Loading

fabiogomessilva Apr 30, 2018 •

edited

Loading

fabiogomessilva Apr 30, 2018 •

edited

Loading

kjin Apr 30, 2018 •

edited

Loading