feat(plugin-document-load): new plugin for document load for web tracer #433

obecny · 2019-10-15T00:07:26Z

Fixes #405
Fixes #438

I have added some performance metrics based on this article
https://developers.google.com/web/fundamentals/performance/navigation-and-resource-timing
I have added ConsoleExporter as discussed in [Plugin] - WebTracer - add "document-load" plugin #405 - if you think it is not needed I can remove it - however it was much easier for me to develop new plugin and to have easy way of seeing result in console. if you against it maybe we can consider keeping it until there is an easy way of using zipkin with the Web Tracer, as now I wasn't able to easily have something like this.
As it wasn't really decided in which namespace the plugin should be I added this as other plugins plugin-*
Updated example of using WebTracer

mayurkale22 · 2019-10-15T00:19:57Z

codecov-io · 2019-10-15T00:22:56Z

Codecov Report

Merging #433 into master will decrease coverage by 0.12%.
The diff coverage is 86.95%.

@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
- Coverage   95.39%   95.26%   -0.13%     
==========================================
  Files         124      125       +1     
  Lines        6141     6292     +151     
  Branches      506      516      +10     
==========================================
+ Hits         5858     5994     +136     
- Misses        283      298      +15

Impacted Files	Coverage Δ
packages/opentelemetry-core/src/common/types.ts	`100% <ø> (ø)`	⬆️
packages/opentelemetry-web/test/WebTracer.test.ts	`0% <0%> (ø)`	⬆️
packages/opentelemetry-web/src/WebTracer.ts	`0% <0%> (ø)`	⬆️
packages/opentelemetry-tracing/src/Span.ts	`100% <100%> (ø)`	⬆️
...ry-tracing/test/export/ConsoleSpanExporter.test.ts	`100% <100%> (ø)`
packages/opentelemetry-core/src/common/time.ts	`94.91% <100%> (-1.7%)`	⬇️
packages/opentelemetry-tracing/test/Span.test.ts	`100% <100%> (ø)`	⬆️
...ckages/opentelemetry-core/test/common/time.test.ts	`100% <100%> (ø)`	⬆️
...elemetry-tracing/src/export/ConsoleSpanExporter.ts	`85.71% <80%> (+48.21%)`	⬆️
... and 7 more

packages/opentelemetry-exporter-console/src/ConsoleExporter.ts

packages/opentelemetry-plugin-document-load/src/documentLoad.ts

packages/opentelemetry-plugin-document-load/package.json

packages/opentelemetry-web/src/WebTracer.ts

…rmance (safari for example)

draffensperger

Very cool!!

I made a bunch of comments, but it's AWESOME to see this coming together.

packages/opentelemetry-core/src/common/time.ts

packages/opentelemetry-plugin-document-load/src/documentLoad.ts

draffensperger · 2019-10-15T15:49:15Z

packages/opentelemetry-plugin-document-load/src/utils.ts

+ * @param obj
+ * @param key
+ */
+export function hasKey<O>(obj: O, key: keyof any): key is keyof O {


I commented about this above too, but can we remove this function by doing the typeof value === 'number' checks above?

it will not work this way with typescript when you use key with forEach

packages/opentelemetry-plugin-document-load/test/documentLoad.test.ts

packages/opentelemetry-web/src/WebTracer.ts

…l to 0

…y-js into plugin-document-load

… or undefined

obecny · 2019-10-21T15:52:20Z

@draffensperger @mayurkale22 updated code to use events instead of spans now, please have a look again

mayurkale22 · 2019-10-21T16:36:21Z

packages/opentelemetry-tracing/src/Span.ts

+  addEvent(
+    name: string,
+    attributes?: types.Attributes,
+    startTime?: number


I think we should use TimeInput instead of number here. Basically, type TimeInput = HrTime | number | Date.

Optional: It would be nice to have this in the separate PR (adding startTime to addEvent).

This also allows passing in performance.now timestamps easily and more accurately as explained in above comment

Does it make sense to change the order with a default value to startTime?

addEvent(name: string, startTime:TimeInput = hrTime(), attributes?: types.Attributes);

I asked previously if this should be on hold until the time for event is added or I should add this here. My understanding was to add this here then and not to hold this PR longer.

Does it make sense to change the order with a default value to startTime?

addEvent(name: string, startTime:TimeInput = hrTime(), attributes?: types.Attributes);

attributes are already there, so it will break all possible places that this is being used as well as any of new code that is already being developed.

It will break the spec too
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#add-events

An Event is defined by the following properties: (Required) Name of the event. (Optional) One or more Attribute. (Optional) Timestamp for the event.

time is on 3rd place.

Not sure what would be benefit besides great chance to break something in existing code

My concern is it would be a problem if you give no attributes but a startTime to the function. Especially from JavaScript where you don't have the type. WDYT?

Hmm not sure if I understand what you mean, can you give some example and what it may cause and how switching startTime with attributes will fix that ?

With current signature, below are the valid scenarios:

span.addEvent('name'); // just a name span.addEvent('name', {key: 'value'}); // name + attributes span.addEvent('name', {key: 'value'}, Date.now()); // name + attributes + time

But, what if you have a startTime but no attributes?

span.addEvent('name', Date.now()) // invalid params
span.addEvent('name', undefined, Date.now()) // valid but have to pass an undefined

Is it a valid problem or no, what do you think?

ok now it is clear, I can simply add detection if 2nd param is a valid type of TimeInput when arguments are 2 only and then treat it as time

packages/opentelemetry-tracing/src/export/ConsoleSpanExporter.ts

draffensperger · 2019-10-21T17:40:48Z

packages/opentelemetry-core/src/common/types.ts

@@ -21,3 +21,9 @@ export enum LogLevel {
  INFO,
  DEBUG,
 }
+
+export interface TimeOriginLegacy {


Looks good, thanks.

packages/opentelemetry-plugin-document-load/src/documentLoad.ts

draffensperger · 2019-10-21T18:07:15Z

packages/opentelemetry-tracing/src/Span.ts

+  addEvent(
+    name: string,
+    attributes?: types.Attributes,
+    startTime?: number


This also allows passing in performance.now timestamps easily and more accurately as explained in above comment

packages/opentelemetry-tracing/src/export/ConsoleSpanExporter.ts

packages/opentelemetry-types/src/trace/span.ts

packages/opentelemetry-web/src/WebTracer.ts

mayurkale22 · 2019-10-21T22:26:03Z

@open-telemetry/node-approvers and @open-telemetry/node-maintainers Please review and approve when you get a chance, thanks :)

draffensperger

This is great!! I've requested a few much more minor changes, but giving a general LGTM pending those. Excited to see this get in soon.

draffensperger · 2019-10-22T19:59:25Z

packages/opentelemetry-plugin-document-load/src/documentLoad.ts

+        parent: rootSpan,
+      }
+    );
+    if (fetchSpan) {


Nice! I really like this fetch span :)

packages/opentelemetry-plugin-document-load/src/documentLoad.ts

packages/opentelemetry-tracing/src/Span.ts

packages/opentelemetry-tracing/src/export/ConsoleSpanExporter.ts

packages/opentelemetry-types/src/trace/span.ts

packages/opentelemetry-web/src/WebTracer.ts

markwolff · 2019-10-22T23:58:12Z

packages/opentelemetry-plugin-document-load/src/documentLoad.ts

+    const rootSpan = this._startSpan(
+      AttributeNames.DOCUMENT_LOAD,
+      PTN.FETCH_START,
+      entries
+    );


I think a component attribute could be set here. There's no specific spec calling for it, but it is required in each other "span" type.

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md#semantic-conventions

added inside _startSpan

markwolff · 2019-10-22T23:58:29Z

packages/opentelemetry-plugin-document-load/src/documentLoad.ts

+ * This class represents a document load plugin
+ */
+export class DocumentLoad extends BasePlugin<unknown> {
+  readonly component: string = 'document-load';


Should this be static?

Or maybe even a module level const?

I made it the same as for example http component. But the node plugins creates a new instance immediately with passing the component name which is not the case for browser. Isn't the component itself already required later the same way as it is with adding component to each span ? At least I have added the component to each span now as attribute.

markwolff · 2019-10-22T23:58:53Z

packages/opentelemetry-plugin-document-load/src/documentLoad.ts

+ */
+export class DocumentLoad extends BasePlugin<unknown> {
+  readonly component: string = 'document-load';
+  readonly version: string = '1';


What does this version mean? Is it required?

opentelemetry-js/packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts

Line 29 in d6f669a

export abstract class BasePlugin<T> implements Plugin<T> {

markwolff · 2019-10-23T00:02:17Z

packages/opentelemetry-plugin-document-load/src/documentLoad.ts

+   */
+  constructor(config: PluginConfig = {}) {
+    super();
+    this._onDocumentLoaded = this._onDocumentLoaded.bind(this);


Alternatively, to get the intended this scoping, does TS complain about making the class member function an arrow function? Or would this go against style guidelines?

markwolff · 2019-10-23T00:08:23Z

packages/opentelemetry-plugin-document-load/src/documentLoad.ts

+    entries: PerformanceEntries
+  ) {
+    // span can be undefined when entries are missing the certain performance - the span will not be created
+    if (typeof span !== 'undefined' && hasKey(entries, performanceName)) {


nit: 'undefined' should be made a constant (probably outside this plugin), (e.g. Constants.Undefined), so that this can be minified better

Would it work to just do span !== undefined instead?

I would hope that smart minifier should be able to hoist out shared constants

(Though I don't know if they actually do!) I guess to me it feels a little weird to try to do that optimization if it could be done better automatically

You are right, I think !== undefined would work here. I think typeof is only needed for global stuff.

For the string constants, uglify won't do any hoisting here, but maybe other bundlers do?

I suspect the Closure compiler can though I have not specifically verified it (see
https://developers.google.com/closure/compiler/docs/api-tutorial3#better)

There is actually a really cool project called Tsickle that can transform TypeScript code into Closure compiler type annotations and then have it feed into the advanced compilation mode for Closure. I would like to eventually see us be able to distribute <script> tag files for the browser that have been fully minified for basic things like the document load instrumentation. That could help people who aren't interested in custom spans but just want a minimally-invasive way of instrumenting their page loads. Same could go for people who just want page load + out of the box user interaction tracing.

with regards to
Alternatively, to get the intended this scoping, does TS complain about making the class member function an arrow function? Or would this go against style guidelines?
this if fine if you add listener in the same function but here the _onDocumentLoaded is being added in patch and removed in unpatch function - that's why the context needs to be bind before so you remove listener correctly

With regards to micro optimisation - I think this should be a part of uglifier not the code itself.
And typeof someVariable === 'undefined' vs someVariable === undefined .

First example will never raise an error the second will raise a ReferenceError if such thing isn't declared. There used to be also some extra error with this in IE (hard to track as when debugging in IE it works fine, when debug is closed it might break). To keep consistency I'm choosing the first option always to avoid possible errors as typeof will never have such issues.

markwolff · 2019-10-23T00:14:02Z

packages/opentelemetry-plugin-document-load/src/types.ts

+/**
+ * Performance metrics
+ */
+export type PerformanceEntries = {


nit: Could this use Partial?

…y-js into plugin-document-load

markwolff

LGTM!

* Prepare release 3.18.0 Signed-off-by: Yuri Shkuro <ys@uber.com> * fix formatting Signed-off-by: Yuri Shkuro <ys@uber.com>

feat(plugin-document-load): new plugin for document load for web tracer

df5ca9a

obecny requested review from bg451, danielkhan, iredelmeier, mayurkale22, OlivierAlbertini, rochdev and vmarchaud as code owners October 15, 2019 00:07

chore: lint

a718b74

mayurkale22 reviewed Oct 15, 2019

View reviewed changes

packages/opentelemetry-exporter-console/src/ConsoleExporter.ts Outdated Show resolved Hide resolved

mayurkale22 reviewed Oct 15, 2019

View reviewed changes

obecny added 5 commits October 15, 2019 14:35

chore: removing unused dependency

fe04870

chore: adding prepare script

4563aa9

chore: cleanup of not used span processor

0c435a0

chore: merging exporter-console into tracing

8301b03

chore: fixing timeOrigin when browser is using older version of perfo…

f6a526e

…rmance (safari for example)

draffensperger reviewed Oct 15, 2019

View reviewed changes

obecny added 10 commits October 15, 2019 18:21

chore: removing @Private

edd9822

chore: cleaning the docs

6752333

chore: using stubs on public instead of private

1d6593d

chore: added explanation when span can be undefined

da85aa4

Merge branch 'master' into plugin-document-load

3aaf306

chore: adding unit test for case when passed "performanceNow" is equa…

2dba249

…l to 0

Merge branch 'master' into plugin-document-load

9c3f616

Merge branch 'plugin-document-load' of github.com:obecny/opentelemetr…

ba49890

…y-js into plugin-document-load

chore: adding unit test for case when passed "performanceNow" is null…

2880dc3

… or undefined

chore: fixing unit test with null

e6abf4e

mayurkale22 approved these changes Oct 15, 2019

View reviewed changes

obecny added 2 commits October 21, 2019 17:26

chore: reformatting

ec5cb9d

chore: merge branch 'master' into plugin-document-load

cb6fe89

mayurkale22 reviewed Oct 21, 2019

View reviewed changes

packages/opentelemetry-tracing/src/export/ConsoleSpanExporter.ts Outdated Show resolved Hide resolved

draffensperger reviewed Oct 21, 2019

View reviewed changes

obecny added 5 commits October 21, 2019 21:30

chore: updating loop

a67db85

chore: changing type for time

ed4a0b5

chore: refactoring loop, updating jsdoc

5090b54

chore: splitting events into 2 spans

8a38ca3

chore: adding possibility of calling addEvent with 2nd param as time

cfcee4a

draffensperger approved these changes Oct 22, 2019

View reviewed changes

obecny and others added 7 commits October 22, 2019 22:41

chore: updating the last event to be "load end"

9bb6197

chore: updating the name for attributes

d1502b3

chore: fixing test

036944a

chore: cleanups

006a467

chore: adding isTimeInput function with unit tests

077eec2

Merge branch 'master' into plugin-document-load

42aae8d

Merge branch 'master' into plugin-document-load

eb30f1a

markwolff reviewed Oct 23, 2019

View reviewed changes

obecny added 4 commits October 23, 2019 14:47

chore: adding component name

734ffea

Merge branch 'plugin-document-load' of github.com:obecny/opentelemetr…

31f8cf0

…y-js into plugin-document-load

chore: adding license and readme

7e4aab1

chore: updating lint and docs jobs to use node12 image in circleci

abf408b

markwolff approved these changes Oct 23, 2019

View reviewed changes

mayurkale22 merged commit b21ed97 into open-telemetry:master Oct 23, 2019

mayurkale22 mentioned this pull request Nov 7, 2019

Propose to add @obecny as the CODEOWNERS. #505

Merged

obecny deleted the plugin-document-load branch July 8, 2020 12:11

lukaswelinder pushed a commit to agile-pm/opentelemetry-js that referenced this pull request Jul 24, 2020

Prepare release 3.18.0 (open-telemetry#433)

4434a3a

* Prepare release 3.18.0 Signed-off-by: Yuri Shkuro <ys@uber.com> * fix formatting Signed-off-by: Yuri Shkuro <ys@uber.com>

feat(plugin-document-load): new plugin for document load for web tracer #433

feat(plugin-document-load): new plugin for document load for web tracer #433

Conversation

obecny commented Oct 15, 2019 • edited Loading

mayurkale22 commented Oct 15, 2019

codecov-io commented Oct 15, 2019 • edited Loading

Codecov Report

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

obecny commented Oct 21, 2019

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

mayurkale22 commented Oct 21, 2019

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

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

obecny Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

obecny Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markwolff left a comment

Choose a reason for hiding this comment

obecny commented Oct 15, 2019 •

edited

Loading

codecov-io commented Oct 15, 2019 •

edited

Loading

obecny Oct 23, 2019 •

edited

Loading

obecny Oct 23, 2019 •

edited

Loading