feat(span-first): add span v2 support for frames tracking#3447
feat(span-first): add span v2 support for frames tracking#3447buenaflor wants to merge 34 commits intofeat/span-firstfrom
Conversation
buenaflor
changed the title
|
| /// Collects frames from [SentryDelayedFramesTracker], calculates the metrics | ||
| /// and attaches them to spans. | ||
| @internal | ||
| class SpanFrameMetricsCollectorV2 implements PerformanceContinuousCollectorV2 { |
There was a problem hiding this comment.
this class is mostly copied from the transaction version, just adapted to span v2
There was a problem hiding this comment.
Would it make sense to use the abstraction InstrumentationSpan instead, or did the logic also change for v2?
There was a problem hiding this comment.
Updated: I updated the impl to make the unified
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/flutter/lib/src/frames_tracking/span_frame_metrics_collector_v2.dart
Outdated
Show resolved
Hide resolved
packages/flutter/lib/src/integrations/frames_tracking_integration.dart
Outdated
Show resolved
Hide resolved
| /// Collects frames from [SentryDelayedFramesTracker], calculates the metrics | ||
| /// and attaches them to spans. | ||
| @internal | ||
| class SpanFrameMetricsCollectorV2 implements PerformanceContinuousCollectorV2 { |
There was a problem hiding this comment.
Would it make sense to use the abstraction InstrumentationSpan instead, or did the logic also change for v2?
packages/flutter/lib/src/frames_tracking/span_frame_metrics_collector_v2.dart
Outdated
Show resolved
Hide resolved
| @override | ||
| Future<void> onSpanFinished(SentrySpanV2 span, DateTime endTimestamp) async { | ||
| return _tryCatch('onSpanFinished', () async { | ||
| if (span is NoOpSentrySpan) { |
There was a problem hiding this comment.
Same as above, probably also a good idea to check for the UnsetSpan and throw, if we do this elsewhere.
There was a problem hiding this comment.
Rather than checking both let's check if (span is! RecordingSentrySpanV2)
| final collector = SpanFrameMetricsCollectorV2(framesTracker, | ||
| resumeFrameTracking: () => widgetsBinding.resumeTrackingFrames(), | ||
| pauseFrameTracking: () => widgetsBinding.pauseTrackingFrames()); | ||
| _collector = collector; |
There was a problem hiding this comment.
We did implement a V2 perfomrce collector, but are not adding it to options, is this correct?
There was a problem hiding this comment.
it's a behaviour difference.
for transactions we add it to the options so we can manually trigger onSpanFinish and onSpanEnded via options.collector.onSpanFinished() but now that we use lifecycle callbacks we dont need to do that anymroe for spanv2 so we dont need to add it to the options
I'll refactor the static version to use lifeecycle callbacks as well
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
packages/flutter/lib/src/integrations/frames_tracking_integration.dart
Outdated
Show resolved
Hide resolved
I checked and probably not, this specific integration requires lots of concrete specific usages, the abstraction wouldn't help here |
packages/flutter/lib/src/frames_tracking/span_frame_metrics_collector_v2.dart
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @override | ||
| bool get finished => _endTimestamp != null; |
There was a problem hiding this comment.
this change is necessary for the lifecycle callbacks to function correctly. we were triggering the callbacks before the endTimestamp was set (which means we never had access to the endTimestamp) but we could not trigger it after because then the span would be immutable to change in setData for example. so this check alone is not enough
packages/flutter/lib/src/frames_tracking/span_frame_metrics_collector_v2.dart
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @override | ||
| bool get isNoop => _span is! RecordingSentrySpanV2; |
There was a problem hiding this comment.
I think this check suffices
| Future<void> _tryCatch(String methodName, Future<void> Function() fn) async { | ||
| try { | ||
| return fn(); | ||
| return await fn(); |
There was a problem hiding this comment.
await here otherwise errors won't be caught in the catch block
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
2 participants
📜 Description
Part of span-first
💡 Motivation and Context
Part of #3334
💚 How did you test it?
Manual, unit/widget test
📝 Checklist
sendDefaultPiiis enabled🔮 Next steps
#skip-changelog