Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(instrumentation-react-native-navigation): Adding new instrumentation library to track navigation changes for React Native apps #2359

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

facostaembrace
Copy link
Member

@facostaembrace facostaembrace commented Jul 30, 2024

Which problem is this PR solving?

A couple of months ago, my team (Embrace) and I were looking for an instrumentation library that creates telemetry data for React Native applications. However, we noticed there was nothing available that adhered to open source standards. We also noticed people in Stack Overflow was asking about something similar.
This instrumentation library exposes two components that can be implemented at the root of React Native applications to create telemetry data that tracks navigation changes for the most popular navigation approaches in this environment.
Instrumentation request here.

#1089

Short description of the changes

  • Introduce a new instrumentation library to kick off a React Native ecosystem.
  • Exposes two components: <NativeNavigationTracker /> that creates spans and track navigation changes for wix/react-native-navigation library, and <NavigationTracker /> that is meant to be used when the application handles the navigation using @react-navigation/native or expo-router.
  • Both components have the ability of adding a minimum set of configurations.
  • Both components receives a custom provider, but if it is not passed it can also create telemetry data using the global one.
  • Configuration for both allows to add static attributes for spans.

@facostaembrace facostaembrace requested a review from a team July 30, 2024 21:12
Copy link

linux-foundation-easycla bot commented Jul 30, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@facostaembrace facostaembrace changed the title feat(instrumentation-react-native-navigation) Adding new instrumentation library to track navigation changes for React Native apps feat:(instrumentation-react-native-navigation) Adding new instrumentation library to track navigation changes for React Native apps Jul 30, 2024
@facostaembrace facostaembrace force-pushed the instrumentation-react-native-navigation branch from 7899a94 to 42ac15b Compare July 30, 2024 21:23
@facostaembrace facostaembrace changed the title feat:(instrumentation-react-native-navigation) Adding new instrumentation library to track navigation changes for React Native apps feat(instrumentation-react-native-navigation): Adding new instrumentation library to track navigation changes for React Native apps Jul 30, 2024
@facostaembrace facostaembrace force-pushed the instrumentation-react-native-navigation branch from 42ac15b to cc3c154 Compare July 30, 2024 21:33
@facostaembrace
Copy link
Member Author

facostaembrace commented Jul 31, 2024

Fyi: still pending to submit the Instrumentation request that introduces this initiative.
Update: done (request here)

@facostaembrace facostaembrace force-pushed the instrumentation-react-native-navigation branch from cc3c154 to f71bc35 Compare August 1, 2024 17:01
@facostaembrace facostaembrace requested a review from a team as a code owner September 23, 2024 21:13
@JamieDanielson JamieDanielson dismissed their stale review October 9, 2024 16:09

Dismissing stale request for changes, PR is ready for review

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was curious about the large diff here, is it all just coming from devDependencies added in plugins/node/instrumentation-react-native-navigation/package.json?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like that, it's was all autogenerated. Started by a fresh state (by just running npm ci at the beginning of the implementation) + adding the devDependencies for the @opentelemenetry/instrumentation-react-native-navigation (and running the proper npm i) results in this big diff here

}), []);

return (
<NavigationTracker ref={navigationRef} provider={provider}>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I seemed to have hit a typing error when following this example:
"NavigationContainerRefWithCurrent<RootParamList> is not assignable to type LegacyRef<INavigationContainer> | undefined"

I was able to ignore and it seemed to function fine but wasn't sure if that was a known issue is something particular to my setup?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! fixed

}
```

If you dig into the attributes, `view.launch` refers to the moment the app is launched. It will be `true` only the first time the app mounts. Changing the status between background/foreground won't modify this attribute. For this case the `view.state.end` is used, and it can contain two possible values: `active` and `background`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is technically a third "inactive" state that's possible on iOS which may be worth calling out here, see: https://reactnative.dev/docs/appstate#app-states

import { useMemo } from 'react';

const consoleStub = {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious if other instrumentations require similar configurable logging, may be useful to pull out into a resuable helper

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a great idea, I would vote for revisit this initiative after this PR is merged to try to isolate this as a test helper

span.current.setAttributes({
// it should create the first span knowing there is not a previous view
[ATTRIBUTES.initialView]: !!isLaunch,
// it should set the view name in case it's useful have this as attr
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// it should set the view name in case it's useful have this as attr
// it should set the view name in case it's useful to have this as an attr

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like this file was accidentally updated

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true. reverting. thanks for catching this

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 93.49112% with 11 lines in your changes missing coverage. Please review.

Project coverage is 90.81%. Comparing base (d0fb135) to head (201d182).

Files with missing lines Patch % Lines
...ct-native-navigation/src/utils/hooks/useConsole.ts 55.55% 8 Missing ⚠️
...n-react-native-navigation/src/utils/spanFactory.ts 94.44% 2 Missing ⚠️
...native-navigation/src/hooks/useAppStateListener.ts 94.11% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2359    +/-   ##
========================================
  Coverage   90.80%   90.81%            
========================================
  Files         169      176     +7     
  Lines        8018     8187   +169     
  Branches     1632     1667    +35     
========================================
+ Hits         7281     7435   +154     
- Misses        737      752    +15     
Files with missing lines Coverage Δ
...navigation/src/hooks/useNativeNavigationTracker.ts 100.00% <100.00%> (ø)
...ative-navigation/src/hooks/useNavigationTracker.ts 100.00% <100.00%> (ø)
...ct-native-navigation/src/utils/hooks/useSpanRef.ts 100.00% <100.00%> (ø)
...-native-navigation/src/utils/hooks/useTracerRef.ts 100.00% <100.00%> (ø)
...native-navigation/src/hooks/useAppStateListener.ts 94.11% <94.11%> (ø)
...n-react-native-navigation/src/utils/spanFactory.ts 94.44% <94.44%> (ø)
...ct-native-navigation/src/utils/hooks/useConsole.ts 55.55% <55.55%> (ø)

... and 1 file with indirect coverage changes

---- 🚨 Try these New Features:

@kndri
Copy link

kndri commented Nov 15, 2024

So @facostaembrace - do you need another maintainer here to get this reviewed? If so I volunteer to help maintain this with you.

@facostaembrace
Copy link
Member Author

So @facostaembrace - do you need another maintainer here to get this reviewed? If so I volunteer to help maintain this with you.

Hi @kndri ! I think I already have maintainers for this new package but all help is well received!
What I need now is an admin approving the workflows for running once again and then hitting the merge button.
But for sure you can take a look and provide feedback if you want!

Thanks for the interest!!

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd like to help keep this moving forward, and want to give you more leeway especially as this is 0.1.0 and it will be valuable to get feedback on how this works. This is the top requested feature in js-contrib.

@facostaembrace @jpmunz we'd like to add you to a triagers team and set a specific path in the CODEOWNERS path for you. We are working on this for other packages as well to ensure folks are pinged and assigned appropriately, as well as given the power to approve PRs (with the green check). To do this we need you to join the OpenTelemetry org.

I am working on some updated guidelines to add to the repo, but in the meantime please follow these guidelines in the community repo for member requirements.

@facostaembrace
Copy link
Member Author

@JamieDanielson I see in the Issue's template that I should mention two sponsors that "meet the sponsor requirements listed in the community membership guidelines". Am right assuming I can tag you + one person from the regular JS SIG meeting? (we can also discuss about this during tomorrow's meeting). Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants