-
-
Notifications
You must be signed in to change notification settings - Fork 222
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: Integrate Grafana Faro #3139
Conversation
✅ Deploy Preview for design-insights ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for oss-insights ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Adds the react integrations and Faro error boundary around the app and initializes the default tracing/web integrations Signed-off-by: John McBride <[email protected]>
pages/_app.tsx
Outdated
useEffect(() => { | ||
if (process.env.NEXT_PUBLIC_FARO_COLLECTOR_URL && process.env.NEXT_PUBLIC_FARO_APP_ENVIRONMENT != "local") { | ||
initGrafanaFaro(); | ||
} | ||
}, []); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure this is doing what I expect it to do: Faro needs to initialize on the client (not the server) so I figured that a userEffect
would get the app to
But it only seems to capture traces/profiles when the App first loads on the /feed
and I see this log go through:
Faro
Faro is already registered. Either add instrumentations, transports etc. to the global faro instance or use the "isolate" property
which makes me think that maybe there's a different way to instantiate it - Investigating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ended up going with a useRef
which now doesn't seem to cause Faro to be re-initilized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do the Faro docs recommend for a React app?
Signed-off-by: John McBride <[email protected]>
pages/_app.tsx
Outdated
@@ -151,13 +165,15 @@ function MyApp({ Component, pageProps }: ComponentWithPageLayout) { | |||
<SessionContextProvider supabaseClient={supabaseClient} initialSession={pageProps.initialSession}> | |||
<PostHogProvider client={posthog}> | |||
<TipProvider> | |||
{Component.PageLayout ? ( | |||
<Component.PageLayout> | |||
<FaroErrorBoundary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is the right place for the error boundary ... their React docs make it seem like this should wrap a rendered <App />
and I'm not seeing errors flow through when testing.
lib/utils/grafana.ts
Outdated
// In the future, we may choose to integrate with the router instrumentation to | ||
// get deeper metrics on matched routes, navigation types, etc. | ||
// Next/router doesn't seem to be supported which won't give us route metrics. | ||
// | ||
// Reference: https://github.com/grafana/faro-web-sdk/tree/main/packages/react | ||
// | ||
// router: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not getting any additional routes besides feed/
in the traces and I think it's because it's due to there note being a router config here (and not really seeing a way for it to support next/router
, only React router)
After experimenting abit with this, we can definitely get this in and start getting some good latency data. But there are some missing integrations in the library with NextJS that i think make it abit half baked. We can draft this for now and I can watch to see when they land some Next integrations. |
…gated up Signed-off-by: John McBride <[email protected]>
const faroRef = useRef<null | Faro>(null); | ||
|
||
useEffect(() => { | ||
if ( | ||
process.env.NEXT_PUBLIC_FARO_COLLECTOR_URL && | ||
process.env.NEXT_PUBLIC_FARO_APP_ENVIRONMENT != "local" && | ||
!faroRef.current | ||
) { | ||
faroRef.current = initGrafanaFaro(); | ||
} | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useRef seems unnecessary. If all you're trying to do is initialize this for the client-side only, this should suffice.
const faroRef = useRef<null | Faro>(null); | |
useEffect(() => { | |
if ( | |
process.env.NEXT_PUBLIC_FARO_COLLECTOR_URL && | |
process.env.NEXT_PUBLIC_FARO_APP_ENVIRONMENT != "local" && | |
!faroRef.current | |
) { | |
faroRef.current = initGrafanaFaro(); | |
} | |
}, []); | |
useEffect(() => { | |
if ( | |
process.env.NEXT_PUBLIC_FARO_COLLECTOR_URL && | |
process.env.NEXT_PUBLIC_FARO_APP_ENVIRONMENT != "local" | |
) { | |
initGrafanaFaro(); | |
} | |
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what I tried initially but would see this pop up in:
Faro
Faro is already registered. Either add instrumentations, transports etc. to the global faro instance or use the "isolate" property
But that must be due to a re-render that happens? Either way, seems to be fine since the points still get sent to the collector endpoint ok:
I'm curious - is the entirety of your app client-side only, or are you doing server-side rendering of components where you're using faro? |
We do have server side rendering. Unfortunately, Faro is not well supported for Next JS yet so we're uncertain the future of this integration. |
Closing as this has grown stale and there is still no adoption in Grafana Faro of NextJS. Let's continue to watch grafana/faro-web-sdk#496 and see if there's any movement there. |
Description
Adds the react integrations and Faro error boundary around the app.
Initializes the default tracing/web integrations for Faro to get us some internal metrics and tracing.
Related Tickets & Documents
N/a - related to ongoing observability and performance work
Mobile & Desktop Screenshots/Recordings
Here's a snapshot after running this locally and getting some of those metrics into our Grafana:
Steps to QA
Tier (staff will fill in)
[optional] Are there any post-deployment tasks we need to perform?
Need to update Netlify env vars to use these
[optional] What gif best describes this PR or how it makes you feel?