-
Notifications
You must be signed in to change notification settings - Fork 8
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: add telemetry #272
feat: add telemetry #272
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
self review
package.json
Outdated
@@ -19,6 +19,7 @@ | |||
"@emotion/react": "^11.10.4", | |||
"@emotion/styled": "^11.10.4", | |||
"@hookstate/core": "^4.0.0-rc21", | |||
"@ipfs-shipyard/ignite-metrics": "https://github.com/ipfs-shipyard/ignite-metrics#feat/useStorageForBrowsers-with-dist", |
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.
will update when ipfs-shipyard/ignite-metrics#64 is merged and new ignite-metrics is released
lib/types.d.ts
Outdated
@@ -157,3 +157,5 @@ export interface IssueDataViewInput { | |||
// isRootIssueLoading: boolean; | |||
// isPendingChildrenLoading: boolean; | |||
} | |||
|
|||
export type BrowserMetricsProvider = import('@ipfs-shipyard/ignite-metrics').BrowserMetricsProvider |
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.
safely getting the type here since importing it is sketchy with nextjs while this package is still cjs.
hooks/useTelemetry.ts
Outdated
import useSharedHook from '../lib/client/createSharedHook'; | ||
|
||
// eslint-disable-next-line react-hooks/rules-of-hooks | ||
const [useTelemetry, setTelemetry] = useSharedHook(useState, null); | ||
|
||
export { useTelemetry, setTelemetry }; |
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.
be able to use telemetry object from anywhere.
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.
a few questions looks good otherwise.
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.
self review
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.
🚀 send it
const { BrowserMetricsProvider } = await igniteMetricsModulePromise | ||
const newTelemetry = new BrowserMetricsProvider({ appKey: '294089175b8268e44bc4e4fab572fe250d57b968' }) | ||
// @ts-expect-error | ||
window.telemetry = newTelemetry |
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.
window hookup can be automatic from the provider. Just wondering if we should add an issue.
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.
it could be useful, but this should be temporary until we add the UI treatments.. i didn't want to block on ignite-metrics any further
depends on ipfs-shipyard/ignite-metrics#64