-
-
Notifications
You must be signed in to change notification settings - Fork 355
Metrics for React Native SDK #5402
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
base: main
Are you sure you want to change the base?
Changes from all commits
e706613
713d146
8ccda2b
d327971
b763d55
8a8c1bf
190a7ba
0a27b75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ export { | |
| setCurrentClient, | ||
| addEventProcessor, | ||
| lastEventId, | ||
| metrics, | ||
| } from '@sentry/core'; | ||
|
|
||
| export { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,180 @@ | ||
| import { getClient, metrics, setCurrentClient } from '@sentry/core'; | ||
| import { ReactNativeClient } from '../src/js'; | ||
| import { getDefaultTestClientOptions } from './mocks/client'; | ||
| import { NATIVE } from './mockWrapper'; | ||
|
|
||
| jest.mock('../src/js/wrapper', () => jest.requireActual('./mockWrapper')); | ||
|
|
||
| const EXAMPLE_DSN = 'https://[email protected]/148053'; | ||
|
|
||
| describe('Metrics', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| jest.useFakeTimers(); | ||
| (NATIVE.isNativeAvailable as jest.Mock).mockImplementation(() => true); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| const client = getClient(); | ||
| client?.close(); | ||
| jest.clearAllTimers(); | ||
| jest.useRealTimers(); | ||
| }); | ||
|
|
||
| describe('beforeSendMetric', () => { | ||
| it('is called when enableMetrics is true and a metric is sent', async () => { | ||
| const beforeSendMetric = jest.fn(metric => metric); | ||
|
|
||
| const client = new ReactNativeClient({ | ||
| ...getDefaultTestClientOptions({ | ||
| dsn: EXAMPLE_DSN, | ||
| enableMetrics: true, | ||
| beforeSendMetric, | ||
| }), | ||
| }); | ||
|
|
||
| setCurrentClient(client); | ||
| client.init(); | ||
|
|
||
| // Send a metric | ||
| metrics.count('test_metric', 1); | ||
|
|
||
| jest.advanceTimersByTime(10000); | ||
| expect(beforeSendMetric).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('is not called when enableMetrics is false', async () => { | ||
| const beforeSendMetric = jest.fn(metric => metric); | ||
|
|
||
| const client = new ReactNativeClient({ | ||
| ...getDefaultTestClientOptions({ | ||
| dsn: EXAMPLE_DSN, | ||
| enableMetrics: false, | ||
| beforeSendMetric, | ||
| }), | ||
| }); | ||
|
|
||
| setCurrentClient(client); | ||
| client.init(); | ||
|
|
||
| // Send a metric | ||
| metrics.count('test_metric', 1); | ||
|
|
||
| jest.advanceTimersByTime(10000); | ||
| expect(beforeSendMetric).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('is called when enableMetrics is undefined (metrics are enabled by default)', async () => { | ||
| const beforeSendMetric = jest.fn(metric => metric); | ||
|
|
||
| const client = new ReactNativeClient({ | ||
| ...getDefaultTestClientOptions({ | ||
| dsn: EXAMPLE_DSN, | ||
| beforeSendMetric, | ||
| }), | ||
| }); | ||
|
|
||
| setCurrentClient(client); | ||
| client.init(); | ||
|
|
||
| // Send a metric | ||
| metrics.count('test_metric', 1); | ||
|
|
||
| jest.advanceTimersByTime(10000); | ||
| expect(beforeSendMetric).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('allows beforeSendMetric to modify metrics when enableMetrics is true', async () => { | ||
| const beforeSendMetric = jest.fn(metric => { | ||
| // Modify the metric | ||
| return { ...metric, name: 'modified_metric' }; | ||
| }); | ||
|
|
||
| const client = new ReactNativeClient({ | ||
| ...getDefaultTestClientOptions({ | ||
| dsn: EXAMPLE_DSN, | ||
| enableMetrics: true, | ||
| beforeSendMetric, | ||
| }), | ||
| }); | ||
|
|
||
| setCurrentClient(client); | ||
| client.init(); | ||
|
|
||
| // Send a metric | ||
| metrics.count('test_metric', 1); | ||
|
|
||
| jest.advanceTimersByTime(10000); | ||
| expect(beforeSendMetric).toHaveBeenCalled(); | ||
| const modifiedMetric = beforeSendMetric.mock.results[0]?.value; | ||
| expect(modifiedMetric).toBeDefined(); | ||
| expect(modifiedMetric.name).toBe('modified_metric'); | ||
| }); | ||
|
|
||
| it('allows beforeSendMetric to drop metrics by returning null', async () => { | ||
| const beforeSendMetric = jest.fn(() => null); | ||
|
|
||
| const client = new ReactNativeClient({ | ||
| ...getDefaultTestClientOptions({ | ||
| dsn: EXAMPLE_DSN, | ||
| enableMetrics: true, | ||
| beforeSendMetric, | ||
| }), | ||
| }); | ||
|
|
||
| setCurrentClient(client); | ||
| client.init(); | ||
|
|
||
| // Send a metric | ||
| metrics.count('test_metric', 1); | ||
|
|
||
| // Advance timers | ||
| jest.advanceTimersByTime(10000); | ||
| expect(beforeSendMetric).toHaveBeenCalled(); | ||
| expect(beforeSendMetric.mock.results[0]?.value).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('metrics API', () => { | ||
| it('metrics.count works when enableMetrics is true', () => { | ||
| const client = new ReactNativeClient({ | ||
| ...getDefaultTestClientOptions({ | ||
| dsn: EXAMPLE_DSN, | ||
| enableMetrics: true, | ||
| }), | ||
| }); | ||
|
|
||
| setCurrentClient(client); | ||
| client.init(); | ||
|
|
||
| expect(() => { | ||
| metrics.count('test_metric', 1); | ||
| }).not.toThrow(); | ||
| }); | ||
|
|
||
| it('metrics can be sent with tags', async () => { | ||
| const beforeSendMetric = jest.fn(metric => metric); | ||
|
|
||
| const client = new ReactNativeClient({ | ||
| ...getDefaultTestClientOptions({ | ||
| dsn: EXAMPLE_DSN, | ||
| enableMetrics: true, | ||
| beforeSendMetric, | ||
| }), | ||
| }); | ||
|
|
||
| setCurrentClient(client); | ||
| client.init(); | ||
|
|
||
| // Send a metric with tags | ||
| metrics.count('test_metric', 1, { | ||
| attributes: { environment: 'test' }, | ||
| }); | ||
|
|
||
| jest.advanceTimersByTime(10000); | ||
| expect(beforeSendMetric).toHaveBeenCalled(); | ||
| const sentMetric = beforeSendMetric.mock.calls[0]?.[0]; | ||
| expect(sentMetric).toBeDefined(); | ||
| }); | ||
| }); | ||
| }); | ||
|
Comment on lines
+1
to
+180
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test suite advances timers by 10 seconds (line 40, 62, etc.) expecting metrics to be sent, but there is no verification that the actual metric data reaches the transport layer or that the metrics aggregation completes. Consider adding assertions on the transport mock or verifying that 🤖 Prompt for AI Agent Did we get this right? 👍 / 👎 to inform future reviews.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point actually |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,6 +118,30 @@ export default function TabOneScreen() { | |
| }} | ||
| /> | ||
| </View> | ||
| <View style={styles.buttonWrapper}> | ||
| <Button | ||
| title="Send count metric" | ||
| onPress={() => { | ||
| Sentry.metrics.count('count_metric', 1); | ||
| }} | ||
| /> | ||
| </View> | ||
| <View style={styles.buttonWrapper}> | ||
| <Button | ||
| title="Send distribution metric" | ||
| onPress={() => { | ||
| Sentry.metrics.count('distribution_metric', 100); | ||
| }} | ||
| /> | ||
| </View> | ||
| <View style={styles.buttonWrapper}> | ||
| <Button | ||
| title="Send count metric with attributes" | ||
| onPress={() => { | ||
| Sentry.metrics.count('count_metric', 1, { attributes: { from_test_app: true } }); | ||
| }} | ||
| /> | ||
| </View> | ||
| <View style={styles.buttonWrapper}> | ||
| <Button | ||
| title="Flush" | ||
|
|
@@ -202,7 +226,7 @@ export default function TabOneScreen() { | |
| Sentry.logger.warn('expo warn log'); | ||
| Sentry.logger.error('expo error log'); | ||
|
|
||
| Sentry.logger.info('expo info log with data', { database: 'admin', number: 123, obj: { password: 'admin'} }); | ||
| Sentry.logger.info('expo info log with data', { database: 'admin', number: 123, obj: { password: 'admin' } }); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated auto-formatting change |
||
| }} | ||
| /> | ||
| </View> | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -57,6 +57,10 @@ Sentry.init({ | |||||||
| logWithoutTracing('Transaction beforeSend:', event.event_id); | ||||||||
| return event; | ||||||||
| }, | ||||||||
| beforeSendMetric(metric: Sentry.Metric) { | ||||||||
| logWithoutTracing('Metric beforeSend:', metric.name, metric.value); | ||||||||
| return metric; | ||||||||
| }, | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wdyt of enabling metrics for the sample app and adding an example in the app (e.g. a button or screen that uses metrics)
Suggested change
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am surprised it is not on the experimental field |
||||||||
| // This will be called with a boolean `didCallNativeInit` when the native SDK has been contacted. | ||||||||
| onReady: ({ didCallNativeInit }) => { | ||||||||
| logWithoutTracing( | ||||||||
|
|
||||||||
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.
We could mention that the feature is beta (similar to feedback and SR in the past)
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.
also a small snippet on how to use it would be nice here
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.
@antonis do we want to keep it as beta since it no longer is in beta for JavaScript?
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 would advocate towards shipping as beta since the product is still labeled as beta in JS sidebar