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.

Sign up for GitHub

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

Jump to bottom

[unified_analytics] Suppress any FileSystemException thrown during Analytics.send #274

Merged
merged 7 commits into from
Jun 5, 2024
Merged

[unified_analytics] Suppress any FileSystemException thrown during Analytics.send #274

merged 7 commits into from
Jun 5, 2024

Conversation

andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Jun 5, 2024 •
edited
Loading

Resolves #273. See title.

  • 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.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 5, 2024

Package publishing

Package Version Status Publish tag (post-merge)
package:unified_analytics 6.1.1 ready to publish unified_analytics-v6.1.1
package:cli_config 0.2.0 already published at pub.dev
package:extension_discovery 2.0.1-wip WIP (no publish necessary)
package:graphs 2.3.2-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@andrewkolos andrewkolos marked this pull request as ready for review June 5, 2024 07:52
andrewkolos
andrewkolos commented Jun 5, 2024
View reviewed changes
pkgs/unified_analytics/lib/src/log_handler.dart
Comment on lines +278 to +291
try {
// When the record count is less than the max, add as normal;
// else drop the oldest records until equal to max
if (records.length < kLogFileLength) {
logFile.writeAsStringSync(content, mode: FileMode.writeOnlyAppend);
} else {
records.add(content);
records = records.skip(records.length - kLogFileLength).toList();

logFile.writeAsStringSync(records.join('\n'));
}
} on FileSystemException {
// Logging isn't important enough to warrant raising a
// FileSystemException that will surprise consumers of this package.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered adding logic that would retry once after purging some number of records, but I'm not sure if the complexity of this would be worth it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's drop it. The log file is way less important than:

  1. ensuring the client tools don't crash
  2. sending analytics

christopherfujino
christopherfujino approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@christopherfujino christopherfujino 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 4321aec into main Jun 5, 2024
6 checks passed
@andrewkolos andrewkolos deleted the discard-fse-from-log-write branch June 5, 2024 18:23
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jun 6, 2024
@devoncarew
[deps] rev crypto, dartdoc, http, logging, test, tools, web_socket_ch…
20996ac 
…annel, webdev, yaml_edit

Revisions updated by `dart tools/rev_sdk_deps.dart`.

crypto (https://github.com/dart-lang/crypto/compare/0a10171..7a9428a):
  7a9428a  2024-06-04  Kevin Moore  Bump lints dep (dart-archive/crypto#172)

dartdoc (https://github.com/dart-lang/dartdoc/compare/45627f9..ddb8fb4):
  ddb8fb44  2024-06-03  Konstantin Scheglov  Use package:analyzer/source/source.dart (dart-lang/dartdoc#3780)

http (https://github.com/dart-lang/http/compare/7bfbeea..a3567f8):
  a3567f8  2024-06-05  Brian Quinlan  Prepare to release cronet_http 1.3.1 (dart-lang/http#1228)
  7addc61  2024-06-05  Hossein Yousefi  [cronet_http] Upgrade jnigen to 0.9.2 to close `#1213` (dart-lang/http#1225)
  eb87a60  2024-06-04  Anikate De  pkgs/ok_http: Close the client (override `close()` from `BaseClient`) (dart-lang/http#1224)
  9f022d2  2024-06-04  Anikate De  pkgs/http_client_conformance_tests: add boolean flag `supportsFoldedHeaders` (dart-lang/http#1222)
  90837df  2024-06-03  Anikate De  pkgs/ok_http: Condense JNI Bindings to `single_file` structure, and add missing server errors test (dart-lang/http#1221)

logging (https://github.com/dart-lang/logging/compare/73f043a..dbd6829):
  dbd6829  2024-06-04  Sarah Zakarias  Update `topics` in `pubspec.yaml` (dart-archive/logging#165)

test (https://github.com/dart-lang/test/compare/2464ad5..83c597e):
  83c597e5  2024-06-05  Jacob MacDonald  enable asserts in dart2wasm tests (dart-lang/test#2241)
  60353804  2024-06-04  Nate Bosch  Remove an unnecessary `dynamic` (dart-lang/test#2239)
  43ad4cd8  2024-06-03  Kevin Moore  random cleanup (dart-lang/test#2232)
  18db77c3  2024-06-02  Kevin Moore  prepare to publish test_api (dart-lang/test#2238)
  cd72e8d8  2024-06-02  Kevin Moore  Prepare to publish (dart-lang/test#2237)
  9c6adaa7  2024-06-01  Kevin Moore  fixes (dart-lang/test#2235)
  0110a80b  2024-06-01  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/test#2236)
  b1b2c029  2024-06-01  Martin Kustermann  Fix `dart run test -p chrome -c dart2wasm` (dart-lang/test#2233)

tools (https://github.com/dart-lang/tools/compare/86b3661..4321aec):
  4321aec  2024-06-05  Andrew Kolos  [unified_analytics] Suppress any `FileSystemException` thrown during `Analytics.send` (dart-lang/tools#274)
  e5d4c8b  2024-06-03  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/tools#271)

web_socket_channel (https://github.com/dart-lang/web_socket_channel/compare/45b8ce9..bf69990):
  bf69990  2024-06-03  Thibault Chatillon  bump web_socket dependency 0.1.3 -> 0.1.5 (dart-lang/web_socket_channel#370)
  5b428dd  2024-06-02  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/web_socket_channel#371)
  afd1e3c  2024-05-23  Brian Quinlan  Remove `--fatal-infos` from `dart pub downgrade` analysis (dart-lang/web_socket_channel#367)
  cb20b71  2024-05-23  Sarah Zakarias  Add `topics` to `pubspec.yaml` (dart-lang/web_socket_channel#362)
  8514229  2024-05-22  Kevin Moore  Bump and fix lints (dart-lang/web_socket_channel#366)

webdev (https://github.com/dart-lang/webdev/compare/a97c2a1..9ada46f):
  9ada46fc  2024-06-05  Nicholas Shahan  Hide more temporary variables when debugging (dart-lang/webdev#2445)

yaml_edit (https://github.com/dart-lang/yaml_edit/compare/963e7a3..08a146e):
  08a146e  2024-06-06  Kavisi  Fix splice list insertion (dart-lang/yaml_edit#84)
  561309e  2024-06-01  dependabot[bot]  Bump actions/checkout from 4.1.5 to 4.1.6 in the github-actions group (dart-lang/yaml_edit#85)
  b582fd5  2024-05-31  Kavisi  Fix error thrown when inserting keys (dart-lang/yaml_edit#80)

Change-Id: I36acbc26fe97d8a1e3fdfa5f4855cc4be7d84dd7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/370080
Commit-Queue: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
andrewkolos added a commit that referenced this pull request Jun 25, 2024
@andrewkolos
[unified_analytics] Suppress any FileSystemException thrown during …
ab5e2da 
…`Analytics.send` (#274)
@andrewkolos andrewkolos mentioned this pull request Jun 25, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christopherfujino christopherfujino christopherfujino approved these changes

Assignees
No one assigned
Labels
package:unified_analytics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[unified_analytics] FileSystemException thrown when unable to write to log file
2 participants
@andrewkolos @christopherfujino