feat(metrics): Add timings method to metrics#12226
Conversation
size-limit report 📦
|
Metrics are only included when performance is included, reducing the base bundle size. We always expose a shim so there is no API breakage, it just does nothing. I noticed this while working on #12226.
packages/core/src/metrics/exports.ts
Outdated
| () => { | ||
| const endTime = timestampInSeconds(); | ||
| const timeDiff = endTime - startTime; | ||
| distribution(aggregator, name, timeDiff, { ...data, unit: 'second' }); |
There was a problem hiding this comment.
I thought about if we should allow to convert this into other units, but for now decided against this - this would just increase bundle size, and not sure what the value would be in doing that? This means that for now this disregards the unit passed in when used in callback form.
There was a problem hiding this comment.
I think it is totally fine being in seconds.
packages/core/src/metrics/exports.ts
Outdated
| () => { | ||
| const endTime = timestampInSeconds(); | ||
| const timeDiff = endTime - startTime; | ||
| distribution(aggregator, name, timeDiff, { ...data, unit: 'second' }); |
There was a problem hiding this comment.
I think it is totally fine being in seconds.
| timing(name: string, value: number, unit?: DurationUnit, data?: Omit<MetricData, 'unit'>): void; | ||
| timing<T>(name: string, callback: () => T, unit?: DurationUnit, data?: Omit<MetricData, 'unit'>): T; |
There was a problem hiding this comment.
Did you check whether this JSDoc shows up properly in VS Code. I believe the JS Doc here might not show up when you use the callback variant.
There was a problem hiding this comment.
I just checked, seems fine for me for both variants:
| // Keeps the frozen DSC on a Sentry Span | ||
| '_frozenDsc', | ||
| // This keeps metrics summary on spans | ||
| '_metrics_summary', |
There was a problem hiding this comment.
Ooops turns out metrics summaries never worked for minified CDN bundles 😬 we had no tests covering this, now we have!
Why doesn't this create an active span? |
I wasn't quite sure what the expected/intended outcome is there 🤔 since this is a bit implicit, not sure what people would want there. but we can make it an active span too, do you think that's better? |
Because it's a callback it feels more intuitive to me to make it active (matches |
|
I updated it to make it an active span! |
AbhiPrasad
left a comment
There was a problem hiding this comment.
cc @krystofwoldrich and @timfish to expose this in React Native and Electron!
This introduces a new method,
metrics.timing(), which can be used in two ways:secondas unit:Closes #12215