feat(instrumentation-react-native-redux): Adding new instrumentation library to track changes during Redux operations #15
Conversation
package.json
Outdated
| ] | ||
| ], | ||
| "dependencies": { | ||
| "react": "^18.0.0" |
There was a problem hiding this comment.
should this one be added in the package folder instead? Is it more of a peer dependency as well?
There was a problem hiding this comment.
yep, good catch! probably it was me hitting npm install react in the wrong place. I will fix it, this file shouldn't have changes
| * happening just once, when the store is created | ||
| */ | ||
| if (!provider) { | ||
| console.info('No TracerProvider found. Using global tracer instead.'); |
There was a problem hiding this comment.
could make configurable like the other PR
There was a problem hiding this comment.
are you talking about the console messages? if so, yes. to make it configurable it's under my plans. if you talk about the Provider, I think it's configured by the following couple of lines
| // @ts-expect-error | ||
| const store = createStore( | ||
| rootReducer, | ||
| compose(applyMiddleware(otelLoggerMiddleware)) |
There was a problem hiding this comment.
so I think a test we could construct to confirm the behaviour when there's other middleware is:
- add some artificial delay of 50ms or something to the redux action
- confirm the span duration for the action is >= 50ms
- apply other middleware before and after the OTEL one that do a noop
- repeat the test and confirm the span still has that duration
facostaembrace
force-pushed
the
facosta/instrumentation-react-native-redux
branch
2 times, most recently
from
5d2a3ed to
073a192
Compare
facostaembrace
changed the title
facostaembrace
force-pushed
the
facosta/instrumentation-react-native-redux
branch
3 times, most recently
from
f8e801c to
18ec2b0
Compare
…ibrary to track changes during Redux operations
facostaembrace
force-pushed
the
facosta/instrumentation-react-native-redux
branch
2 times, most recently
from
c16a316 to
77204f1
Compare
reducing slowness
facostaembrace
force-pushed
the
facosta/instrumentation-react-native-redux
branch
from
77204f1 to
836def0
Compare
jpmunz
left a comment
jpmunz
left a comment
There was a problem hiding this comment.
changes look good to me, I think the only thing missing is a hook for custom attributes so that we can set emb.type from the Embrace SDK
jpmunz
force-pushed
the
facosta/instrumentation-react-native-redux
branch
from
1f72858 to
e2df6b0
Compare
|
@facostaembrace fyi I ran into package conflicts after getting the latest from main, I found it easiest to move forward by removing these dependencies which meant taking the react components out of the tests but I think they still exercise the same functionality: |
Which problem is this PR solving?
Short description of the changes