[EMBR-3872] Instrumentation library(ies) for RN navigation #28
Conversation
| @@ -0,0 +1,4 @@ | |||
| ## OTel Instrumentation Library - React Native / Navigation | |||
There was a problem hiding this comment.
could add a few usage examples as well similar to something like https://github.com/open-telemetry/opentelemetry-js/tree/main/experimental/packages/opentelemetry-instrumentation-http to show how the different instrumentations would be used in an app
given we're introducing the "experimental" folder for the first time I think a quick packages/experimental/README.md would be worthwhile as well, we can chat on slack about what should go in there
|
|
||
| useNativeNavigationTracker(ref, tracer); | ||
|
|
||
| return <>{children}</>; |
There was a problem hiding this comment.
would just return children be equivalent?
There was a problem hiding this comment.
Yes, it would be equivalent but typescript complains if I do not add the fragment as wrapper here. It seems like the typescript version doesn't work well with the types from the current react-native version (it's a very old one, v0.56.0).
| }); | ||
|
|
||
| navigationElRef.registerCommandListener(() => { | ||
| // NOTE: To implement |
There was a problem hiding this comment.
what would we be looking to listen for beyond the element appear/disappear?
There was a problem hiding this comment.
Nothing, you are right. I removed this block of code.
| const tracerRef = useRef<Tracer | null>(null); | ||
|
|
||
| // using the layout effect to make sure the tracer is initialized before the component is rendered | ||
| useEffect(() => { |
There was a problem hiding this comment.
given the comment was this meant to be using useLayoutEffect?
There was a problem hiding this comment.
yes, I changed it for testing purposes but I just added useLayoutEffect back
| // using the layout effect to make sure the tracer is initialized before the component is rendered | ||
| useEffect(() => { | ||
| if (tracerRef.current === null && provider) { | ||
| trace.setGlobalTracerProvider(provider); |
There was a problem hiding this comment.
I think we should avoid this in the library since it may have an unintended effect on the rest of the app's instrumentation, should be able to do provider.getTracer(...) without setting it to be global
| @@ -0,0 +1,44 @@ | |||
| { | |||
| "name": "navigation", | |||
| "version": "1.0.0", | |||
There was a problem hiding this comment.
maybe "0.1.0" since we're keeping under experimental for now
| "version": "1.0.0", | ||
| "description": "Instrumentation library that captures telemetry data during navigation changes.", | ||
| "author": "Embrace.io", | ||
| "license": "UNLICENSED", |
There was a problem hiding this comment.
Apache-2.0 to match the top level
| }, | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "[email protected]/embrace-io/embrace-io.git" |
There was a problem hiding this comment.
| "instrumentation" | ||
| ], | ||
| "bugs": { | ||
| "url": "https://github.com/embrace-io/embrace-io/issues" |
There was a problem hiding this comment.
| "@types/react-test-renderer": "^18.0.7" | ||
| }, | ||
| "keywords": [ | ||
| "otel", |
There was a problem hiding this comment.
I'd add "opentelemetry" spelled out as well, e.g. https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/exporter-logs-otlp-http/package.json#L51
| NavigationTrackerProps | ||
| >(({children, provider}, ref) => { | ||
| // Initializing a Trace instance | ||
| const tracer = useTrace({name: 'navigation', version: '1.0.0'}, provider); |
There was a problem hiding this comment.
you may be able to read the name + version from package.json actually
| // starts the initial span | ||
| expect(mockStartSpan).toHaveBeenCalledTimes(1); | ||
|
|
||
| mockGetCurrentRoute.mockReturnValue({name: 'second-view-test'}); |
There was a problem hiding this comment.
I'd be curious to take another look at these tests with less pieces mocked, e.g. I think we could use an actual tracerProvider and just need to supply it with a exporter that is mocked and in that way assert on the full body of the spans that would have been written out
efaaeb6
to
8d1f089
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
8d1f089
to
bfa64da
Compare
8c30b00
to
dc4a003
Compare
| @@ -3,7 +3,7 @@ | |||
| "target": "es6", | |||
| "module": "commonjs", | |||
| "moduleResolution": "node", | |||
| "jsx": "react", | |||
| "jsx": "react-native", | |||
There was a problem hiding this comment.
this is necessary for running tests that implement components
experimental/navigation/README.md
Outdated
| import {NavigationTracker} from '@embrace/react-native/experimental/navigation'; | ||
|
|
||
| const App: FC = () => { | ||
| const navigationRef = useNavigationContainerRef(); // if you not use `expo-router` the same hook is also available in `@react-navigation/native` since `expo-router` is built on top of it |
There was a problem hiding this comment.
| const navigationRef = useNavigationContainerRef(); // if you not use `expo-router` the same hook is also available in `@react-navigation/native` since `expo-router` is built on top of it | |
| const navigationRef = useNavigationContainerRef(); // if you do not use `expo-router` the same hook is also available in `@react-navigation/native` since `expo-router` is built on top of it |
experimental/navigation/README.md
Outdated
|
|
||
| const App: FC = () => { | ||
| const navigationRef = useNavigationContainerRef(); // if you not use `expo-router` the same hook is also available in `@react-navigation/native` since `expo-router` is built on top of it | ||
| const provider = useProvider(); // the provider is something you need to configurate and pass down as prop into the `NavigationTracker` component |
There was a problem hiding this comment.
| const provider = useProvider(); // the provider is something you need to configurate and pass down as prop into the `NavigationTracker` component | |
| const provider = useProvider(); // the provider is something you need to configure and pass down as prop into the `NavigationTracker` component |
| type NavigationTrackerRef = NavRef; | ||
| interface NavigationTrackerProps { | ||
| children: ReactNode; | ||
| // selected provider, should be configured by the app consumer |
There was a problem hiding this comment.
we could make this optional to simplify the setup and instruct the user to set it only if they don't wish to use the global one, then by default if it's not set we can do:
import {trace} from "@opentelemetry/api";
trace.getTracer(name, version)
which defaults to the globally set tracerprovider or a no-op provider if there isn't one set: https://github.com/open-telemetry/opentelemetry-js/blob/main/api/src/api/trace.ts#L88
There was a problem hiding this comment.
👍 README looks great, I would just suggest:
- I think that first sentence would be good as a quick overview in
experimental/README.mdas well to note that all packages in this folder are under development - Maybe include a section on expected telemetry that this will output, e.g. a span for each view that ends when a new view is reached, etc.
| @@ -32,6 +32,16 @@ buildscript { | |||
|
|
|||
There was a problem hiding this comment.
I'll add this to the KTLO ticket on package development, I was noticing these extra lines added on my branch as well
| @@ -53,6 +53,7 @@ | |||
| "react-native": ">=0.56.0" | |||
| }, | |||
| "dependencies": { | |||
| "@opentelemetry/api": "^1.9.0", | |||
There was a problem hiding this comment.
maybe ties into the work on the next instrumentation or tech debt, is there a way these dependencies can just be added to experimental/navigation/package.json?
There was a problem hiding this comment.
+1. I think I'll revisit it as a separated ticket (one into the KTLO list, as you mentioned). but totally agreed on it
| // using the layout effect to make sure the tracer is initialized before the component is rendered | ||
| useLayoutEffect(() => { | ||
| if (tracerRef.current === null && provider) { | ||
| tracerRef.current = trace.getTracer(name, version); |
There was a problem hiding this comment.
should it be provider.getTracer when provider is truthy?
| @@ -0,0 +1,18 @@ | |||
| interface TNavigationContainer { | |||
There was a problem hiding this comment.
I haven't seen this convention used with 'T' before, should it be 'I' instead?
There was a problem hiding this comment.
yeah, I saw in the past that people used T as prefix when describing a shape for typescript (but declaring that as a type). I think in here it would make sense to update the name in favor of the I prefix since this is declared using interface instead
| return; | ||
| } | ||
|
|
||
| spanEnd(span); |
There was a problem hiding this comment.
I forget if we already discussed this but we should think about how we want to handle the last view when the app ends or gets backgrounded. If we don't have a strong opinion maybe just adding a function to the hook's return value like endLastSpan or something and telling the user that if they care about ending it they should hook it up to a handler
There was a problem hiding this comment.
we talked about it, but we didn't decide any approach. I like the suggestion, I'll dig into this on friday
4eb6d98
to
2673882
Compare
…(testing purposes)
…r experimental folder
01a31ab
to
5d0482f
Compare
Goal
Introduce new instrumentation library that creates telemetry data under "experimental" mode that tracks changes into the navigation.
Pending: