feat: Add React Plugin #164

thgao · 2020-07-27T20:38:19Z

Which problem is this PR solving?

React/Redux plugin #84

Short description of the changes

Adding plugin for React instrumentation + docs and tests
This plugin instruments the lifecycle methods of a React component as defined here: https://projects.wojtekmaj.pl/react-lifecycle-methods-diagram/
Note: Constructor & getDerivedStateFromProps are not instrumented because they are static methods. getDerivedStateFromProps is considered an extremely rare use case

Usage

Once the tracer is defined, statically set logger & tracer:

BaseOpenTelemetryComponent.setLogger(logger);
BaseOpenTelemetryComponent.setTracer("name", "version");

Component definitions:

class export Component1 extend BaseOpenTelemetryComponent { ...}
class export Component2 extend BaseOpenTelemetryComponent { ...}

Any component extending BaseOpenTelemetryComponent will be instrumented.

Example Traces

Example Attributes:

Mounting Flow:

Updating Flow:

Unmounting Flow:

- changed plugin model to component based approach - fixed tests

codecov · 2020-07-27T20:43:37Z

Codecov Report

Merging #164 into master will increase coverage by 0.16%.
The diff coverage is 97.11%.

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
+ Coverage   94.06%   94.23%   +0.16%     
==========================================
  Files          74       78       +4     
  Lines        3536     3744     +208     
  Branches      374      395      +21     
==========================================
+ Hits         3326     3528     +202     
- Misses        210      216       +6

Impacted Files	Coverage Δ
...s/web/opentelemetry-plugin-react-load/.eslintrc.js	`0.00% <0.00%> (ø)`
...lugin-react-load/src/BaseOpenTelemetryComponent.ts	`97.47% <97.47%> (ø)`
...etry-plugin-react-load/src/enums/AttributeNames.ts	`100.00% <100.00%> (ø)`
...web/opentelemetry-plugin-react-load/src/version.ts	`100.00% <100.00%> (ø)`

markwolff · 2020-07-29T14:26:09Z

plugins/web/opentelemetry-plugin-react-load/README.md

+BaseOpenTelemetryComponent.setTracer('name', 'version');
+```
+
+To instrument components, extend `BaseOpenTelemetryComponent`:


How can I instrument a functional component? Would I just wrap it as a HOC?

const FuncComponent = ({...props}) => (<BaseOpenTelemetryComponent> <div {...props}>Hello World</div> </BaseOpenTelemetryComponent>);

Right now it doesn't support functional components, I was hoping this was okay because functional components don't have lifecycle methods. If we wanted to add tracing to it I was thinking that could be a separate strain of work to instrument useEffect. I was also thinking this would be okay since functional components are typically less complicated than class components so it would likely be more important for a user to want the class component to be instrumented, what do you think?

Most of components that we write today are functional components since react hooks make class-based components unnecessary in most cases. As such, instrumentation of functional components will be a big plus.

plugins/web/opentelemetry-plugin-react-load/package.json

markwolff · 2020-07-29T14:48:13Z

plugins/web/opentelemetry-plugin-react-load/src/BaseOpenTelemetryComponent.ts

+          parentData.name = AttributeNames.MOUNTING_SPAN;
+        }
+        return plugin._instrumentFunction(this, 'render', () => {
+          return original!.apply(this, args);


Is the assertion here required?

Related to the comment below! The type of the original function can be undefined as specified by React, but in my code I guarantee it to be a no-op when undefined. This patch function has to have the same signature as the actual React method so that the shimmer wrap function can take them as arguments

markwolff · 2020-07-29T14:50:40Z

plugins/web/opentelemetry-plugin-react-load/src/types.ts

+/*
+ * method "componentDidMount" from React.Component
+ */
+export type ComponentDidMountFunction = (() => void) | undefined;


Since you are providing your own default no-ops for these, can the | undefined and non-falsy assertions! be removed?

I had to add | undefined in order to match the function definition provided by React, otherwise when I tried to wrap a function using Shimmer it didn't compile since the types of the patch I'm supplying and actual function prototype passed in don't match

markwolff · 2020-07-29T14:54:11Z

plugins/web/opentelemetry-plugin-react-load/src/BaseOpenTelemetryComponent.ts

+        const apply = plugin._instrumentFunction(
+          this,
+          'componentDidMount',
+          () => {


nit: these can all be written without defining new arrow functions as original.bind(this, args), but I'm not sure if it matters either way

markwolff · 2020-07-29T15:05:29Z

plugins/web/opentelemetry-plugin-react-load/src/BaseOpenTelemetryComponent.ts

+  private _getAttributes(react: React.Component) {
+    let state = '';
+    try {
+      state = JSON.stringify(react.state);


@open-telemetry/javascript-maintainers
Do attributes need to be non-object types? I wonder if this stringify is something that should be left to the exporters. Else I think the attribute value type should be changed from unknown to clarify whether a stringify is necessary or not.

We already have an issue to track this on our side open-telemetry/opentelemetry-js#1340, and a spec PR which resolved this open-telemetry/opentelemetry-specification#722, but the simple answer is that this is not allowed:

The attribute key, which MUST be a non-null and non-empty string.

The attribute value, which is either:

A primitive type: string, boolean, double precision floating point (IEEE 754-1985) or signed > 64 bit integer.

An array of primitive type values. The array MUST be homogeneous,
i.e. it MUST NOT contain values of different types. For protocols that do
not natively support array values such values SHOULD be represented as JSON strings.

plugins/web/opentelemetry-plugin-react-load/src/BaseOpenTelemetryComponent.ts

markwolff · 2020-07-29T15:13:00Z

examples/react-load/src/Home.jsx

+import { Link } from 'react-router-dom';
+import { BaseOpenTelemetryComponent } from '@opentelemetry/plugin-react-load';
+
+class Home extends BaseOpenTelemetryComponent {


Is another example using hooks/functional components also possible with this plugin?

Related to comment above, but currently my implementation doesn't support functional components

dyladan · 2020-07-29T15:52:51Z

Has this been tested with preact? Would be interested to see if it works, and would be a nice addition to the README.

dyladan · 2020-07-29T19:19:59Z

plugins/web/opentelemetry-plugin-react-load/src/BaseOpenTelemetryComponent.ts

+      [GeneralAttribute.COMPONENT]: this.moduleName,
+      [AttributeNames.LOCATION_URL]: window.location.href,
+      [AttributeNames.REACT_NAME]: react.constructor.name,
+      [AttributeNames.REACT_STATE]: state,


Is this a requirement? I think the state can become quite large in some cases and this may cause quite some overhead.

I added in state as an attribute because I found it a good way to decipher where/when spans were coming from in an application, would it be better to not include this attribute or only conditionally include it based on length?

A length heuristic is likely confusing for users. Maybe you could define a protected property on the base class like getCustomSpanAttributes: undefined | (state: State) => Attributes, which is called with state iff it is defined, and its returned attributes are attached to the span? Or a simpler property like addStateAsSpanAttribute: boolean might be easier... It depends what use-case you're actually trying to solve.

If the use-case is to help you debug while building the module, I would say to strip it out before merge. If it is actually impossible to tell which component generated the span, then maybe there is some other attribute that could cover that use-case like the component's constructor.name property.

I see, I'm trying to solve the user side use case. I was thinking that since it's common in React to have multiple state updates in a row (for example setting setting the state to Loading, then setting it to Loaded once a request returns), these would all come from the same component so just having a name (which is an attribute right now too) might still make it hard for users to decipher the traces from each other.
Could you clarify about your suggestion I think I'm misunderstanding it a bit, is the suggestion to still allow long states to be added as attributes but not add the state label at all when state is undefined?

@dyladan @thgao was this addressed already or there are still things that left ?

I haven't addressed this change yet, I was hoping to understand it a bit more and get some clarification first!

obecny

Overall great work and I really like it :). I have tested this heavily and I posted all issues / bugs that I found.
There are still things which doesn't work fully. The main problem is with context propagation between other plugins and this plugin. I have posted you some fixes and files which when you modify it will be easier to debug things.

Findings:

Preact
Without fixes
https://imgur.com/dBWRdmX
With fixes
https://imgur.com/xEVDoom
React
Without fixes
https://imgur.com/i4tTpO4
With fixes
https://imgur.com/78z7lDo

Summary

Preact looks like this is working fine after my fix with context propagation.
React is still having some problem, although the XMLHttpRequest is getting a correct parent,
but as you can see you spans are splitted into 2 groups and also the click event is not a parent for start group.
Why those 2 groups are not connected whereas in preact it works fine
WIth regards to click event for me it looks like the react might either have some addition life cycle that the plugin is not taking into consideration or something else.

Finally the react should look like preact and I'm enclosing you the image how it should look like
https://imgur.com/SpLw3nQ

examples/react-load/preact/.gitignore

examples/react-load/preact/package.json

examples/react-load/preact/public/index.html

examples/react-load/preact/size-plugin.json

examples/react-load/preact/src/web-tracer.js

examples/react-load/preact/package.json

examples/react-load/preact/src/components/app.js

examples/react-load/react/package.json

examples/react-load/react/README.md

plugins/web/opentelemetry-plugin-react-load/src/BaseOpenTelemetryComponent.ts

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>

thgao · 2020-08-04T20:33:18Z

React is still having some problem, although the XMLHttpRequest is getting a correct parent,
but as you can see you spans are splitted into 2 groups and also the click event is not a parent for start group.
Why those 2 groups are not connected whereas in preact it works fine
WIth regards to click event for me it looks like the react might either have some addition life cycle that the plugin is not taking into consideration or something else.

Finally the react should look like preact and I'm enclosing you the image how it should look like
https://imgur.com/SpLw3nQ

After investigating a bit, I found that if the button handler is added directly using addEventListener like:

 document.getElementById('id').addEventListener('click', () => this.buttonHandler(this))

instead of setting the handler using the onClick method as part of the button, the React traces get nested under the the click span as expected.

As discussed, we think this is likely due to some React optimizations or a bug in zone.js, and out of scope of this PR.

obecny

great job

thgao added 20 commits July 23, 2020 14:37

feat(opentelemetry-plugin-react-load): instruments mounting

d728c14

test(opentelemetry-plugin-react-load): added tests for mounting spans

00480bd

refactor(opentelemetry-plugin-react-load): cleaned up types

557cbd6

feat(opentelemetry-plugin-react-load): added attributes

05a259e

feat(opentelemetry-plugin-react-load): updating flow spans + tests

2d7fe04

fix(opentelemetry-plugin-react-load): root of codecov test command fix

3bdaa50

fix(opentelemetry-plugin-react-load): make attr names snake case

3ce9c36

fix(opentelemetry-plugin-react-load): removed logs from test components

97fe0d6

feat(opentelemetry-plugin-react-load): unmounting instrument

2697443

feat(opentelemetry-plugin-react-load): base func for error handling

504bd0d

refactor(opentelemetry-plugin-react-load): working component approach

5869d25

- changed plugin model to component based approach - fixed tests

fix(opentelemetry-plugin-react-load): fixed js docs

6a3ffd6

fix(opentelemetry-plugin-react-load): fixed error span naming and bug

cd8c580

fix(opentelemetry-plugin-react-load): use version

980a564

style(opentelemetry-plugin-react-load): cleaned up typos

d41cf17

style(opentelemetry-plugin-react-load): set up eslint + ran linter

6d0bb8c

test(opentelemetry-plugin-react-load): unmounting test

4d53551

style(opentelemetry-plugin-react-load): ran linter

ee7d473

fix(opentelemetry-plugin-react-load): jsonified state stringify error

7aae7ef

docs(opentelemetry-plugin-react-load): react plugin docs & example

ab0bb37

thgao requested a review from a team July 27, 2020 20:38

thgao changed the title ~~React plugin~~ feat: Add React Plugin Jul 27, 2020

markwolff reviewed Jul 29, 2020

View reviewed changes

markwolff requested a review from a team July 29, 2020 15:23

fix(opentelemetry-plugin-react-load): make logs warn & fix react depend

ef3604f

dyladan reviewed Jul 29, 2020

View reviewed changes

docs(opentelemetry-plugin-react-load): preact example

47335ab

style(opentelemetry-plugin-react-load): eof newlines

57f693c

obecny reviewed Jul 31, 2020

View reviewed changes

thgao and others added 7 commits July 31, 2020 10:33

fix: Update examples/react-load/react/README.md

589aa1b

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>

fix: context propagation for other plugins

a0658c3

fix: removed unsed version variable

cf97370

style: redundant variable initializer

0278307

refactor: name parent spans when starting span

14c4925

style: clean up react load examples/docs

cc5bf08

docs: added docker to react load examples

9e37ecd

thgao requested review from obecny and markwolff August 4, 2020 20:33

obecny approved these changes Aug 5, 2020

View reviewed changes

obecny mentioned this pull request Aug 5, 2020

onclick - react, zone.js #168

Closed

dyladan added the Internship Ending Soon label Aug 5, 2020

markwolff approved these changes Aug 5, 2020

View reviewed changes

dyladan added the enhancement New feature or request label Aug 5, 2020

dyladan approved these changes Aug 12, 2020

View reviewed changes

obecny merged commit 5be5217 into open-telemetry:master Aug 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add React Plugin #164

feat: Add React Plugin #164

thgao commented Jul 27, 2020

codecov bot commented Jul 27, 2020 •

edited

Loading

markwolff Jul 29, 2020

thgao Jul 29, 2020

panfiva Sep 23, 2020

markwolff Jul 29, 2020

thgao Jul 29, 2020

markwolff Jul 29, 2020

thgao Jul 29, 2020

markwolff Jul 29, 2020

markwolff Jul 29, 2020 •

edited

Loading

dyladan Jul 29, 2020

markwolff Jul 29, 2020

thgao Jul 29, 2020

dyladan commented Jul 29, 2020

dyladan Jul 29, 2020

thgao Jul 29, 2020

dyladan Jul 29, 2020

dyladan Jul 29, 2020

thgao Jul 29, 2020

obecny Aug 7, 2020

thgao Aug 7, 2020

obecny left a comment

thgao commented Aug 4, 2020

obecny left a comment

feat: Add React Plugin #164

feat: Add React Plugin #164

Conversation

thgao commented Jul 27, 2020

Which problem is this PR solving?

Short description of the changes

Usage

Example Traces

codecov bot commented Jul 27, 2020 • 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

Choose a reason for hiding this comment

markwolff Jul 29, 2020 • 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

dyladan commented Jul 29, 2020

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 left a comment

Choose a reason for hiding this comment

thgao commented Aug 4, 2020

obecny left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 27, 2020 •

edited

Loading

markwolff Jul 29, 2020 •

edited

Loading