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

[unified_analytics 5.8.8] Prevent FileSystemExceptions from happening when logging outgoing events #280

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Jun 25, 2024

The goal of this is to patch unified_analytics v5 with the fixes from #274 and #277.

In summary, unified_analytics logs (the 2500 most-recent) outgoing analytics events. This is primarily to be read in the future for targeted surveys. However, due to some unknown edge case(s) (investigation pending), it's possible this file can reach a massive size. #277 fixed this for unified_analytics v6, but we need to port this to v5, because the stable branch of Flutter (which we want to hotfix) uses v5.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

andrewkolos and others added 2 commits June 25, 2024 11:33
* delete old log file if it exceeds kMaxLogFileSize of 25MiB

* update changelog

* bump version in pubspec.yaml

* update constants.dart
@andrewkolos andrewkolos changed the title [unified_analytics 5.8.8] Prevent FileSystemExceptions from happening when logging outgoing payloads [unified_analytics 5.8.8] Prevent FileSystemExceptions from happening when logging outgoing events Jun 25, 2024
@andrewkolos
Copy link
Contributor Author

@devoncarew My intention here is to create a patch for unified_analytics v5 (5.8.8+2). Here's what I've done so far/plan to do:

Let me know if you think this won't work.

@andrewkolos andrewkolos marked this pull request as ready for review June 25, 2024 19:57
@andrewkolos
Copy link
Contributor Author

It looks like CI only watches the main branch, so I'm not sure what the workflow would be here.

@devoncarew
Copy link
Member

This broadly makes sense. A few notes:

  • I don't see a tag on this repo for 5.8.8+1 but do see a pub publish for it; we should tag the commit that was published from for posterity
  • wrt versioning, we're publishing a new bug fix to 5.8.8; that could just as easily be versioned as 5.8.9 instead of 5.8.8+2 but in practice either version will work
  • the publishing automation will want to be publishing from main - it won't handle publishing from a branch. Once this is merged into a new 5.8.x branch - and all the content is ready to go - I can manually publish from that branch. Alternatively I could give you publishing permissions to tools.dart.dev

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewkolos andrewkolos merged commit 6617807 into unified_analytics-v5.8.8+2 Jun 25, 2024
2 checks passed
@andrewkolos andrewkolos deleted the 588-log-file-patch branch June 25, 2024 20:12
@andrewkolos
Copy link
Contributor Author

andrewkolos commented Jun 25, 2024

Oops, I probably should have ran the tests before merging 😛. I'll probably just fix the new branch directly.

@devoncarew
Copy link
Member

The tests should have run for this PR automatically.

Looking at the config, it looks like its set up to only run for PRs which target main: https://github.com/dart-lang/tools/blob/main/.github/workflows/unified_analytics.yml; definitely something we should update.

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 1, 2024
…8+1 to 5.8.8+2 (#151046)

Resolves #150137 (the issue is already resolved in master with the use of package:unified_analytics:6.1.2).

Patches the fixes from [unified_analytics:5.8.8+2](dart-lang/tools#280) to stable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants