Codecov Report

❌ Patch coverage is 93.75000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.58%. Comparing base (2b5e090) to head (7980011).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3257      +/-   ##
==========================================
+ Coverage   88.22%   89.58%   +1.36%     
==========================================
  Files         294      100     -194     
  Lines       10209     3553    -6656     
==========================================
- Hits         9007     3183    -5824     
+ Misses       1202      370     -832     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Android Performance metrics 🚀

iOS Performance metrics 🚀

@stefanosiano Same here, review pls :)

looks good, but i'll put it on hold, until the last point has been decided on: https://www.notion.so/sentry/Logs-Replays-Notes-2738b10e4b5d8035b027c6cb0fc57174
TLDR: when replay is in buffering mode, we'd send a replay id even if the replay is not sent. There is a discussion on what to do

@denrase decision was made
We are going to add a sentry._internal.replay_is_buffering if replay is in buffer mode, and we don't send it if replay is not sampled or in session mode.

Then we keep the sentry.replay_id flag as you already did in this PR 👍

@stefanosiano Pls take a look if i interpreted the flag correctly.

@denrase
The logic should be this:

1. Session replay is running -> set sentry.replay_id
2. Buffer replay is running -> set sentry.replay_id and sentry._internal.replay_is_buffering
3. No replay is running -> don't set anything

In Android we have this distinction because the replay id is set in the scope when the replay is captured (and it's in session mode), and is only available in the replay integration (and not in the scope) when it's running in buffer mode.
Can we do something similar here?
Also, why are you creating a new integration if you can check it directly in the client in captureLog()?

@stefanosiano Thank you, didn't have access to SentryFlutterOptions in captureLog

Thank you, didn't have access to SentryFlutterOptions in captureLog

i guess it's fine if you try to cast the options of the scope to SentryFlutterOptions.
At least the logic of enhancing the logs is not scattered in different classes, imho

@stefanosiano AFAIK this only works in the Flutter repo, as the Dart repo does not know about the SentryFlutterOptions class at all.

@stefanosiano AFAIK this only works in the Flutter repo, as the Dart repo does not know about the SentryFlutterOptions class at all.

yeah right

@stefanosiano Took me a while to debug this and figure out how everything fits together. We have the following issues:

• On iOS we read the replay ID from the Scope, but do not have access to an internal/integration ID, so we cannot reliably tell if we are in buffer mode. This is called continuously for every screenshot.

• On Android we get the replay ID from the integration, which we set to the scope, so it's immediately set, even when in buffer mode. This is called once when the recorder is starting.

So the latter is currently incorrect I think. Need to think about how we wan't to tackle this.

\cc @buenaflor

@denrase sounds like we need some internal hybrids SDK api from cocoa for this

I think it might be good to align with RN here

@buenaflor Checked RN. Right now they are only setting the replay_id without buffering info.

https://github.com/getsentry/sentry-react-native/blob/0b647531c13884b6be72c6f652482d79c6d2d96f/packages/core/src/js/integrations/logEnricherIntegration.ts#L91

@buenaflor Checked RN. Right now they are only setting the replay_id without buffering info.

is sentry._internal.replay_is_buffering a strict requirement right now? or should we move forward without it and handle this in another PR?

@buenaflor On Android this works as expected AFAIK, on iOS we can only detect full sessions or onError sessions as soon as they are set to the scope. @stefanosiano Do you know if this would be ok as a step between? RN also only sets replay_id without buffering info, though i did not check if this is already released.

Generated by 🚫 dangerJS against 7980011