Skip to content
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

Validation for fetchedDisplayRefreshRate #2299

Closed
wants to merge 2 commits into from

Conversation

Volsavr
Copy link

@Volsavr Volsavr commented Sep 19, 2024

📜 Description

Additional validation for displayRefreshRate received from the native level.

💡 Motivation and Context

We noticed that following code:

final fetchedDisplayRefreshRate = await _native?.displayRefreshRate();

sometimes returns 0. It happens only on macOS Monterey and below (At least in our cases).

Assigning 0 to fetchedDisplayRefreshRate leads to exception:

Unsupported operation: Infinity or NaN toInt #0 SpanFrameMetricsCollector.calculateFrameMetrics (package:sentry_flutter/src/span_frame_metrics_collector.dart) #1 SpanFrameMetricsCollector.onSpanFinished (package:sentry_flutter/src/span_frame_metrics_collector.dart:80) #2 SentrySpan.finish (package:sentry/src/protocol/sentry_span.dart:81) #3 SentryTracer.finish (package:sentry/src/sentry_tracer.dart:139) #4 SentryTracer._finishedCallback (package:sentry/src/sentry_tracer.dart:282) #5 SentrySpan.finish (package:sentry/src/protocol/sentry_span.dart:94)

Also it may lead to ui issues in the app (we faced with issues related to displaying svg resources).

💚 How did you test it?

Using default displayRefreshRate (60) prevents exception appearing and ui issues.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

@stefanosiano
Copy link
Member

Hi @Volsavr thanks for opening the PR!
Would you mind adding a test in span_frame_metrics_collector_test.dart?
We need that to avoid regressions in the future.
You can copy the test captures metrics with display refresh rate of 60 if native refresh rate is null replacing null with 0, possibly with a comment on macOS Monterey returning 0.
Then i'll be happy to approve it!

@buenaflor
Copy link
Contributor

I don't think we should set it to a default refresh rate. a couple weeks ago we decided that we want to keep this accurate or not do it so if we can't return a proper refresh rate it should just disable the instrumentation instead of applying a default rate

I can prepare a PR for fixing that

@buenaflor
Copy link
Contributor

We have added validations here #2307

@buenaflor buenaflor closed this Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants