Tracer web #334

obecny · 2019-09-24T22:00:33Z

Which problem is this PR solving?

It adds the skeleton for the tracer-web using BasicTracer , fixes Add tracer implementation for the browser #276
It adds karma runner for BasicTracer and fixes them to pass in karma too

Short description of the changes

I have also added some helper example to be able to debug things directly in browser - this might stay for future use or I can remove. It was helpful during development to better understand what is happening.

… - for development purposes

codecov-io · 2019-09-24T22:05:02Z

Codecov Report

Merging #334 into master will decrease coverage by 2.19%.
The diff coverage is 3.36%.

@@            Coverage Diff            @@
##           master     #334     +/-   ##
=========================================
- Coverage    97.9%   95.71%   -2.2%     
=========================================
  Files         106      108      +2     
  Lines        5213     5154     -59     
  Branches      431      433      +2     
=========================================
- Hits         5104     4933    -171     
- Misses        109      221    +112

Impacted Files	Coverage Δ
packages/opentelemetry-tracer-web/src/WebTracer.ts	`0% <0%> (ø)`
...elemetry-tracer-web/test/StackScopeManager.test.ts	`0% <0%> (ø)`
...es/opentelemetry-tracer-web/test/WebTracer.test.ts	`0% <0%> (ø)`
...kages/opentelemetry-tracer-basic/test/Span.test.ts	`100% <100%> (ø)`	⬆️
...ges/opentelemetry-exporter-zipkin/src/transform.ts	`100% <0%> (ø)`	⬆️
.../opentelemetry-exporter-zipkin/test/zipkin.test.ts	`100% <0%> (ø)`	⬆️
...pe-async-hooks/test/AsyncHooksScopeManager.test.ts	`100% <0%> (ø)`	⬆️
...entelemetry-exporter-zipkin/test/transform.test.ts	`100% <0%> (ø)`	⬆️
...ackages/opentelemetry-exporter-zipkin/src/utils.ts
... and 4 more

packages/opentelemetry-basic-tracer/karma.conf.js

packages/opentelemetry-basic-tracer/package.json

packages/opentelemetry-tracer-web/karma.conf.js

packages/opentelemetry-basic-tracer/package.json

packages/opentelemetry-core/src/platform/index.ts

draffensperger · 2019-09-24T23:48:34Z

packages/opentelemetry-tracer-web/README.md

+[![devDependencies][devDependencies-image]][devDependencies-url]
+[![Apache License][license-image]][license-image]
+
+This module provides automatic tracing for Node.js applications.


Could you update this readme to talk about basic tracing for the web browser?

draffensperger · 2019-09-24T23:49:42Z

packages/opentelemetry-tracer-web/examples/js/index.js

@@ -0,0 +1,34 @@
+import { WebTracer } from '../../src';
+
+import * as shimmer from 'shimmer';


Although I think use of shimmer is cool/useful in automatic tracing contexts, would it make sense for the example to be a little more basic and just show how to e.g. create a span for some code that does a timeout or XHR call?

I have used it as some playground to debug stuff and understand this better :). Wasn't really sure what to show as an example and how many of them we should have. Maybe we can talk a bit more to have similar examples in other places too?

Yeah, I think this is OK for now. In the future, we can always improve the examples in various ways.

draffensperger · 2019-09-24T23:50:49Z

packages/opentelemetry-tracer-web/src/StackScopeManager.ts

+
+import { ScopeManager } from '@opentelemetry/scope-base';
+
+export class StackScopeManager implements ScopeManager {


Could you add some documentation for this class about how it works? In particular, to highlight its limitations in terms of not keeping context across async calls?

Related to that, what's the value of the StackScopeManager here over a simpler model where it just stores a single scope at a time? I think storing a single scope will work for basic web cases such as the initial load or even user interactions where we assume only one thing is happening at a time.

For concurrent user interactions, I think we really need something that is async-aware like a Zone.js based scope manager - but that's a more advanced use case.

With regards to value of StackScopeManager over simpler model. Can you please provide what you mean by simpler model ?. The BasicTracer expects to define a scopeManager with certain functionality:
https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-basic-tracer/src/types.ts#L63.
So I would say that StackScopeManager is exactly the simpler model , unless I'm missing something. The more advanced would be the one that will cover the async functionality as you said.

What if you had a ScopeManager implementation that looked like this:

export interface ScopeManager { private _scope: unknown = {}; active(): unknown { return this._scope; } with<T extends (...args: unknown[]) => ReturnType<T>>( scope: unknown, fn: T ): ReturnType<T> { this._scope = scope; return fn.apply(args); } bind<T>(target: T, scope?: unknown): T { if (scope !== undefined) { this._scope = scope; } return target; } }

That to me feels like the most minimal scope manager that actually does something: it stores the scope that you give it in a single variable and makes no attempt to propagate it or make it hierarchical or async in any way. (Of course it's missing some things like documentation, error handling, etc. but gives the idea)

That would be sufficient for some browser use cases (e.g. the initial document load) where we really just have one global scope.

Sorry for the slow response on this! I think I missed your reply an earlier time I looked at this.

Alternatively, here is a slightly more complex version of with that captures the scope and restores it without needing an explicit data structure (could do the same for bind):

with<T extends (...args: unknown[]) => ReturnType<T>>( scope: unknown, fn: T ): ReturnType<T> { const oldScope = this._scope; return (...args) => { this._scope = scope; try { return fn.apply(args); } finally { this._scope = oldScope; } }; }

This still feels a little lighter weight, but I think I would prefer the most simple version where we just have one scope at a time explicitly since we don't do any async scope propagation, and then have the Zone.js based one do the fancy propagation and hierarchical wrapping.

@draffensperger ok I have simplified the scope manager. Previously I had identified the scopes by adding some of ids to them. It was also to be able to identify them later in async but that is not the case anymore as we will rather use the new scope manager for that something like ZoneScopeManager. I have also realised that there might be multiple tracer so adding id to scope is not the best idea. Keeping it shortly then I would leave the StackScopeManager and treat it as really the basic one as in your example there would be a simple problem with things like calling the callback with scope inside another callback - it would not be restored after all.

Yeah, that makes sense to want to make sure we can at least restore scope for simple callbacks.

Is there a way you can use closures (function wrapping via arrow functions) to restore the scope rather than using UIDs and an object? I think that might be a little bit cleaner and use less code, but maybe there's a use case I'm not understanding here

@draffensperger extra closure is not needed in this case , the fn is a callback function that is called immediately with the current scope, there is no waiting for that like with bind function. Unless I'm not aware of something.

Ah, I see. I guess then bind would need storage via the closure, and with could just store it in a local variable?

Between those techniques is it possible to change _scopesStack to just an _scope variable and simplify the logic?

@draffensperger yes I was just referring to the extra closure as I wasn't sure if I'm not missing anything, the rest is fine, just pushed the changes you can have a look, thx

Thanks for the refactoring! I will make a few more comments, but they are more stylistic. I think this is really coming together well!

packages/opentelemetry-tracer-web/src/StackScopeManager.ts

packages/opentelemetry-tracer-web/webpack/test.config.js

packages/opentelemetry-tracer-web/.gitignore

mayurkale22 · 2019-09-25T00:50:27Z

packages/opentelemetry-tracer-web/.npmignore

@@ -0,0 +1,4 @@
+/bin


Instead of opentelemetry-tracer-web, what do you think about opentelemetry-web-tracer?

this name has been already decided in OpenTelem web draft I don't have strong opinion on that but personally I would keep opentelemetry-tracer-web, opentelemetry-tracer-node, opentelemetry-tracer-basic

@mayurkale22 so what do you think about it ?, I'm fine for both, but I didn't propose opentelemetry-tracer-web although thinking this name to be easier as we can have then the namespace of opentelemetry-tracer-*

I am fine with opentelemetry-tracer-web, which means we need to rename opentelemetry-basic-tracer -> opentelemetry-tracer-basic.

packages/opentelemetry-tracer-web/package.json

mayurkale22 · 2019-09-25T00:55:24Z

packages/opentelemetry-basic-tracer/package.json

  "types": "build/src/index.d.ts",
  "repository": "open-telemetry/opentelemetry-js",
  "scripts": {
-    "test": "nyc ts-mocha -p tsconfig.json test/**/**/*.ts",
+    "test": "nyc ts-mocha -p tsconfig.json 'test/**/*.ts' --exclude 'test/index-webpack.ts'",


I think we have already excluded test/index-webpack.ts in .nycrc. https://github.com/open-telemetry/opentelemetry-js/blob/master/.nycrc#L11

if I remove it then it doesn't work

mayurkale22 · 2019-09-25T00:55:45Z

packages/opentelemetry-tracer-web/package.json

+  "types": "build/src/index.d.ts",
+  "repository": "open-telemetry/opentelemetry-js",
+  "scripts": {
+    "test": "nyc ts-mocha -p tsconfig.json 'test/**/*.ts' --exclude 'test/index-webpack.ts'",


Same comment here about .nycrc.

packages/opentelemetry-tracer-web/src/StackScopeManager.ts

packages/opentelemetry-tracer-web/src/WebTracer.ts

packages/opentelemetry-basic-tracer/package.json

packages/opentelemetry-tracer-web/tslint.json

packages/opentelemetry-tracer-web/package.json

…tracer-web

…nged recently

draffensperger

Excited for this - great job @obecny on getting this tracer working for the web!

I think the architecture is there, just made a few more comments around types, styling, etc.

draffensperger · 2019-10-03T13:03:25Z

packages/opentelemetry-tracer-web/src/StackScopeManager.ts

+  /**
+   * Keeps the reference to current scope
+   */
+  public _currentScope: any;


Since we aren't actually doing anything with the scope, can we use unknown here?

draffensperger · 2019-10-03T13:07:26Z

packages/opentelemetry-tracer-web/src/StackScopeManager.ts

+      writable: false,
+      value: target.length,
+    });
+    return contextWrapper as any;


If you do contextWrapper as unknown as T can you remove the any here and in the const contextWrapper: any = above?

draffensperger · 2019-10-03T13:08:01Z

packages/opentelemetry-tracer-web/src/StackScopeManager.ts

+   * @param target
+   * @param scope
+   */
+  bind<T>(target: T | any, scope?: unknown): T {


Would it be possible to make the type of target only be T?

draffensperger · 2019-10-03T13:08:26Z

packages/opentelemetry-tracer-web/src/StackScopeManager.ts

+   * @param fn Callback function
+   */
+  with<T extends (...args: unknown[]) => ReturnType<T>>(
+    scope: any,


Can scope be unknown since we aren't doing anything with its contents?

draffensperger · 2019-10-03T13:09:14Z

packages/opentelemetry-tracer-web/src/StackScopeManager.ts

+   */
+  with<T extends (...args: unknown[]) => ReturnType<T>>(
+    scope: any,
+    fn: any


Can fn be () => (scope: unknown) to specify that it will be called with a single argument of the scope value?

when fn is a mentioned type then I see ts lint errors,
would the following be fine ?

with<T extends (...args: unknown[]) => ReturnType<T>>( scope: unknown, fn: () => ReturnType<T> ): ReturnType<T> { if (typeof scope === 'undefined' || scope === null) {

Yes, that's great!

draffensperger · 2019-10-03T13:12:10Z

packages/opentelemetry-tracer-web/test/StackScopeManager.test.ts

+import { StackScopeManager } from '../src';
+
+// @ts-ignore
+if (typeof global.window === 'undefined') {


I would think the opentelemetry-tracer-web tests would only ever be meant to run in a browser context, not a Node context.

Given that, could you modify package.json to make it so that running the Node tests is just a no-op so we can remove this? As a minor other pro to that approach, the CI build will save a bit of time from not having to run these tests in Node.

draffensperger · 2019-10-03T13:14:36Z

packages/opentelemetry-tracer-web/test/StackScopeManager.test.ts

+      return done();
+    });
+
+    it('should finally restore an old scope', done => {


Nice to see this tested and working!

draffensperger · 2019-10-03T13:14:49Z

packages/opentelemetry-tracer-web/test/StackScopeManager.test.ts

+      });
+      assert.strictEqual(scopeManager.active(), window);
+    });
+    it('should finally restore an old scope when scope is an object', done => {


Nit: should we put a newline before this it block?

draffensperger

💯

mayurkale22 · 2019-10-04T19:31:36Z

Looks like this PR is ready, would you like to include this in the alpha release?

obecny · 2019-10-04T19:38:35Z

@mayurkale22 I think yes

mayurkale22 · 2019-10-04T19:49:24Z

Could you please remove "private": true and make sure all the versions (tracer-web, examples, etc) are pointing to 0.1.0?

obecny · 2019-10-04T19:57:08Z

not sure what you mean by removing "private": true ?
make sure all the versions (tracer-web, examples, etc) are pointing to 0.1.0?
you mean the dependencies from package.json ?

  "dependencies": {
    "@opentelemetry/core": "^0.1.0",
    "@opentelemetry/scope-base": "^0.1.0",
    "@opentelemetry/types": "^0.1.0",
    "@opentelemetry/tracer-basic": "^0.1.0"
  }

or changing the "version": "0.0.1" to "version": "0.1.0" and then updating examples/tracer-web/package.json to "@opentelemetry/tracer-web": "^0.1.0" ?

mayurkale22 · 2019-10-04T20:52:14Z

Sorry for the confusion. Please ignore the 1st point (I thought it is present in tracer-web's package.json).

For 2nd point, changing the "version": "0.0.1" to "version": "0.1.0" and then updating examples/tracer-web/package.json to "@opentelemetry/tracer-web": "^0.1.0" is what I expect.

danielkhan

Happy to have this in the alpha

…e span (open-telemetry#334) * tchannelbridge error logging fixed Signed-off-by: Varsha <[email protected]> * Test added for error logging Signed-off-by: Varsha <[email protected]> * tchannel err test modified Signed-off-by: Varsha <[email protected]>

* feat(tracer-web): adding tracer web * feat(basic-tracer): adding karma tests * feat(tracer-web): adding some example for easier debugging in browser - for development purposes * fix: lint * fix: creating base for karma * fix: fixing problem with target for browser, cleanup tests * refactor: moving polyfills for node karma tests to one file * fix: adding missing package * refactor: removing unneeded file * refactor: prefixing privates, cleanup * fix: duplicate package * refactor: aligning tslint with other tslint packages * refactor: cleanups, adding comments for class * fix: linting * fix: type * refactor: generation of id for scope * refactor: removed previous uid for scope as originally it was meant to be used with async which is not the case anymore * chore: adding test for restoring scope * fix: lint * refactor: simplifying the stack scope manager * chore: updating readme with basic example * chore: fixes after merge * fix: updating test to accept greater or equal - fails on browser * refactor: moving example for web tracer * refactor: removing WebTracerConfig to use BasicTracerConfig which changed recently * chore: updating types * chore: spacing * chore: removing mocha tests for tracer-web * chore: updating types and linting * chore: updating packages after merge * chore: adding nyc report for karma tests for browser * chore: updating lerna script to run coverage for browsers * feat(tracer-web): bump version to 0.1.0

Co-authored-by: Bartlomiej Obecny <[email protected]> Co-authored-by: Valentin Marchaud <[email protected]>

obecny added 3 commits September 24, 2019 23:49

feat(tracer-web): adding tracer web

d9d152d

feat(basic-tracer): adding karma tests

ab74943

feat(tracer-web): adding some example for easier debugging in browser…

8cfea09

… - for development purposes

obecny requested review from bg451, danielkhan, iredelmeier, mayurkale22, OlivierAlbertini, rochdev and vmarchaud as code owners September 24, 2019 22:00

obecny added 2 commits September 25, 2019 00:15

fix: conflicts with master

873a287

fix: lint

9e37fb9

markwolff reviewed Sep 24, 2019

View reviewed changes

draffensperger reviewed Sep 24, 2019

View reviewed changes

obecny added 2 commits September 25, 2019 02:35

fix: creating base for karma

d10fbdd

fix: fixing problem with target for browser, cleanup tests

62965ad

mayurkale22 reviewed Sep 25, 2019

View reviewed changes

Merge branch 'master' into tracer-web

354930d

OlivierAlbertini suggested changes Sep 29, 2019

View reviewed changes

packages/opentelemetry-basic-tracer/package.json Outdated Show resolved Hide resolved

packages/opentelemetry-tracer-web/tslint.json Outdated Show resolved Hide resolved

packages/opentelemetry-tracer-web/package.json Show resolved Hide resolved

obecny added 9 commits September 30, 2019 15:28

refactor: moving polyfills for node karma tests to one file

e8b7503

fix: adding missing package

117c10f

refactor: removing unneeded file

d5129ca

refactor: prefixing privates, cleanup

8eca21f

fix: duplicate package

74941c1

refactor: aligning tslint with other tslint packages

a7616f1

refactor: cleanups, adding comments for class

62406aa

fix: merge branch 'master' into tracer-web

757458c

Merge branch 'tracer-web' of github.com:obecny/opentelemetry-js into …

2ff8c62

…tracer-web

OlivierAlbertini approved these changes Sep 30, 2019

View reviewed changes

obecny added 2 commits October 3, 2019 01:15

refactor: moving example for web tracer

50ec127

refactor: removing WebTracerConfig to use BasicTracerConfig which cha…

8f6ae2c

…nged recently

mayurkale22 approved these changes Oct 3, 2019

View reviewed changes

draffensperger reviewed Oct 3, 2019

View reviewed changes

obecny added 8 commits October 3, 2019 21:09

chore: updating types

104ba4b

chore: spacing

b0eb537

chore: removing mocha tests for tracer-web

04eced8

chore: updating types and linting

e90b49f

chore: merge branch 'master' into tracer-web

a57fdc8

chore: updating packages after merge

5df0726

chore: adding nyc report for karma tests for browser

61fc904

chore: updating lerna script to run coverage for browsers

d0579b9

draffensperger approved these changes Oct 4, 2019

View reviewed changes

obecny mentioned this pull request Oct 4, 2019

Add karma tests for basic-tracer #133

Closed

obecny added 2 commits October 4, 2019 22:58

feat(tracer-web): bump version to 0.1.0

1dfc09f

Merge branch 'master' into tracer-web

92d5694

danielkhan approved these changes Oct 4, 2019

View reviewed changes

Merge branch 'master' into tracer-web

3091a89

mayurkale22 merged commit 82b5fad into open-telemetry:master Oct 7, 2019

mayurkale22 mentioned this pull request Oct 11, 2019

Add karma tests to basic tracer #280

Closed

mayurkale22 mentioned this pull request Nov 7, 2019

Propose to add @obecny as the CODEOWNERS. #505

Merged

obecny deleted the tracer-web branch July 8, 2020 12:16

		@@ -0,0 +1,34 @@
		import { WebTracer } from '../../src';

		import * as shimmer from 'shimmer';


		import { ScopeManager } from '@opentelemetry/scope-base';

		export class StackScopeManager implements ScopeManager {

Tracer web #334

Tracer web #334

Conversation

obecny commented Sep 24, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov-io commented Sep 24, 2019 • edited Loading

Codecov Report

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

draffensperger Sep 30, 2019 • edited Loading

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

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

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

Choose a reason for hiding this comment

obecny Oct 3, 2019 • edited Loading

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

draffensperger left a comment

Choose a reason for hiding this comment

mayurkale22 commented Oct 4, 2019

obecny commented Oct 4, 2019

mayurkale22 commented Oct 4, 2019

obecny commented Oct 4, 2019

mayurkale22 commented Oct 4, 2019

danielkhan left a comment

Choose a reason for hiding this comment

obecny commented Sep 24, 2019 •

edited

Loading

codecov-io commented Sep 24, 2019 •

edited

Loading

draffensperger Sep 30, 2019 •

edited

Loading

obecny Oct 3, 2019 •

edited

Loading