-
-
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?
Conversation
� Conflicts: � CHANGELOG.md
5ce970d to
d327971
Compare
| 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' } }); |
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.
Unrelated auto-formatting change
|
@sentry review |
| 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(); | ||
| }); | ||
| }); | ||
| }); |
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 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 NATIVE.sendEnvelope was called with the expected metric envelope. Additionally, ensure that ReactNativeClient properly inherits metrics support from @sentry/core Client.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/core/test/metrics.test.ts#L1-L180
Potential issue: 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 `NATIVE.sendEnvelope` was called with
the expected metric envelope. Additionally, ensure that `ReactNativeClient` properly
inherits metrics support from `@sentry/core` Client.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3443368
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.
good point actually
| beforeSendMetric(metric: Sentry.Metric) { | ||
| logWithoutTracing('Metric beforeSend:', metric.name, metric.value); | ||
| return metric; | ||
| }, |
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.
Wdyt of enabling metrics for the sample app and adding an example in the app (e.g. a button or screen that uses metrics)
| }, | |
| }, | |
| enableMetrics: true, |
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 am surprised it is not on the experimental field
|
|
||
| ### Features | ||
|
|
||
| - Adds metrics ([#5402](https://github.com/getsentry/sentry-react-native/pull/5402)) |
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.
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
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.
Awesome work @alwx 🎸
Left a few comments/suggestions but overall looks great!
|
Sentry docs mention this for JavaScript EDIT: getsentry/sentry-docs#15632 Lets keep it as is |
This looks like an oversight in the docs since we don't mention the experimental elsewhere https://docs.sentry.io/platforms/javascript/metrics/ 🤔 |
|
Thank you for the PR! |

📢 Type of change
📜 Description
Fixes #5384
This PR does the following:
Sentry.metrics,enableMetricsflag andbeforeSendMetriccallback work;💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps