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

[Rollouts]Writing Rollouts to persistence #12300

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

themiswang
Copy link
Contributor

@themiswang themiswang commented Jan 18, 2024

Base on this summary of events crashlytics record:

Events API Persistence File(s) Will Crash?
non-fatal recordError() error_a/b No
native-fatal N/A exception, signal, mach_exception YES
custom exception recordExceptionModel() custom_exception_a/b No
on-demand fatal recordOnDemandExceptionModel() exception(if we consider it's fatal), custom_exception_a/b(if we consider it's non-fatal) No

Here is how we read rollouts info:

File SDK log for rollouts Backend Process
error_a/b (.clsrecord) each error entry will attach the rollouts state with the entry read rollouts from each entry
exception, signal, mach_exception (.clsrecord) write rollouts separately - rollout.clsrecord read rollouts from rollout.clsrecord
custom_exception (.clsrecord) for recordExceptionModel() we add rollouts to each exception entry, for on-demand fatal it will read rollouts from rollout.clsrecord for each entry check if has rollouts attach, if YES then read rollouts info from entry, if NO, read rollouts from rollout.clsrecord

Changes include in this PR:

  • write rollouts to rollouts.clsrecord
  • write rollouts to error.clsrecord
  • write rollouts to custom_exception.clsrecord
  • unit tests

#no-changelog

if let rolloutsData =
getRolloutsStateEncodedJsonData() {
persistenceDelegate.updateRolloutsStateToPersistence(
rollouts: rolloutsData,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why we're using NSData instead of NSString for this parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh because [NSFileHandle writerData:] need NSData as parameter. The other place for non-fatal I passed as NSString is because the helpers need input as NSString...

Copy link
Contributor

@samedson samedson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like how you've split out the swift and ObjC parts using a delegate 😎. Just had some questions. Feel free to ping me again when you want another look!

@@ -271,6 +279,7 @@ void FIRCLSExceptionRecord(FIRCLSExceptionType type,

// Create new report and copy into it the current state of custom keys and log and the sdk.log,
// binary_images.clsrecord, and metadata.clsrecord files.
// also copy rollouts.clsrecord if applicable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious question: is this happening implicitly? I don't see any change to implement it (at least I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah if you the the line below, this is actually copy all the file from the one report directory to another. So if the original report contain rollouts then it will be copy to another report directory using the existing logic.

  BOOL copied = [fileManager.underlyingFileManager copyItemAtPath:currentReportPath
                                                           toPath:newReportPath
                                                            error:&error];

Copy link
Contributor

@danasilver danasilver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM but definitely deferring to Sam to review this one :)

@themiswang themiswang merged commit 4b0c998 into featureRollouts Jan 23, 2024
54 of 55 checks passed
@themiswang themiswang deleted the rolloutsSerialization branch January 23, 2024 19:44
@firebase firebase locked and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants