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

New event added for sending analytics within package on errors #229

Conversation

eliasyishak
Copy link
Contributor

@eliasyishak eliasyishak commented Jan 29, 2024

Fixes:

This PR adds logging to try blocks found within the Session and LogHandler classes. We still have more try blocks within the SurveyHandler class but that will require a refactor to allow us to pass the Analytics.send method. More details in the issue below:


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

Comment on lines +169 to +172
bool legacyOptOut({required FileSystem fs, required Directory home}) {
// List of Maps for each of the config file, `key` refers to the
// key in the json file that indicates if the user has been opted
// out or not
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This big diff is mostly a refactor to not repeat ourselves 3 times (and possibly more as we add more tools) to check the legacy opt out files

@eliasyishak eliasyishak marked this pull request as ready for review February 1, 2024 20:21
@dart-lang dart-lang deleted a comment from github-actions bot Feb 2, 2024
@@ -160,17 +161,23 @@ class LogHandler {
final FileSystem fs;
final Directory homeDirectory;
final File logFile;
final Analytics _analyticsInstance;

/// Ensure we are only sending once per invocation for this class.
Copy link
Member

Choose a reason for hiding this comment

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

Why only send once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the log handlers case, we are checking if each log record is malformed. So in the worst case scenario, we could have 2,500 malformed log records. This check would it make it so that we don't receive tons of errors logs from just one client

Code from log handler below where send these events

    final records = logFile
        .readAsLinesSync()
        .map((String e) {
          try {
            return LogItem.fromRecord(jsonDecode(e) as Map<String, Object?>);
          } on FormatException catch (err) {
            if (!_errorEventSent) {
              _sendFunction(Event.analyticsException(
                workflow: 'LogFileStats.logFileStats',
                error: err.runtimeType.toString(),
                description: 'message: ${err.message}\nsource: ${err.source}',
              ));
              _errorEventSent = true;
            }
            return null;
            // ignore: avoid_catching_errors
          } on TypeError catch (err) {
            if (!_errorEventSent) {
              _sendFunction(Event.analyticsException(
                workflow: 'LogFileStats.logFileStats',
                error: err.runtimeType.toString(),
              ));
              _errorEventSent = true;
            }
            return null;
          }
        })
        .whereType<LogItem>()
        .toList();

Copy link
Member

Choose a reason for hiding this comment

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

What is an "invocation" in this instance? For an analysis server session that lasts 8 hours, would we only ever send one error event, or is the LogHandler not that long-lived?

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 that is a good point, as it stands right now, it would be per dart process. In that case, i can rework the logic for _errorEventSent so that it gets reset every x minutes. The main purpose of this was so that one client wasn't reporting hundreds of the same error event

Copy link
Member

Choose a reason for hiding this comment

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

Rather than using timestamps, can we rather group together operations that we expect could error multiple times, and track a single local bool errorAlreadySent for those operations?

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 think it might be even better to create an abstraction for handling errors, something like ErrorHandler that can be passed to each class created by Analytics.

This class can keep track of what events we have sent for errors and block any events we have already sent. That should centralize our error handling to one class


late int _sessionId;
late int _lastPing;

/// Ensure we are only sending once per invocation for this class.
Copy link
Member

Choose a reason for hiding this comment

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

Again, why only once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same idea for the session handler here except we aren't dealing with the loop from the map method. With the session handler, if we have a problem with the _refreshSessionData method, then it will end up sending an error event every time we call that method, which is every time we send an event.

So if a user had 100 events sent while an instance of Analytics is alive, then we will also have 100 error events

Comment on lines +9 to +13
class ErrorHandler {
/// Stores each of the events that have been sent to GA4 so that the
/// same error doesn't get sent twice.
final Set<Event> _sentErrorEvents = {};
final SendFunction _sendFunction;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christopherfujino thoughts?

Comment on lines -380 to -384
// Using a try/except here to parse out the fields if possible,
// if not, it will quietly return null and won't get processed
// downstream
try {
// Parse out values from the top level key = 'events' and return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having this try/catch block was not necessary since we are already catching the same two exceptions when we call this method

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.

nit about adding another test, otherwise LGTM

@eliasyishak eliasyishak merged commit 8323b21 into dart-lang:main Feb 13, 2024
6 checks passed
@eliasyishak eliasyishak deleted the 167-new-event-for-internal-usage-within-package-for-logging-errors-unifiedanalyticserror branch February 13, 2024 20:45
error: err.runtimeType.toString(),
description: 'message: ${err.message}\nsource: ${err.source}',
));

parseContents();
Copy link
Member

Choose a reason for hiding this comment

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

Won't this just throw again and not be caught?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this was naively assuming that the error we encounter while parsing the json was user related and not coming from the package, creating a fix for this now

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Feb 21, 2024
…, sse, stream_channel, tools, vector_math, web, webdev, yaml_edit

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

async (https://github.com/dart-lang/async/compare/24266ca..6cdbc41):
  6cdbc41  2024-02-15  Kevin Moore  Update to latest lints, require Dart 3.2 (dart-archive/async#267)

browser_launcher (https://github.com/dart-lang/browser_launcher/compare/74a0efe..7956230):
  7956230  2024-02-16  sigmundch  Add extra flags to disable throttling behavior. (dart-archive/browser_launcher#55)

dartdoc (https://github.com/dart-lang/dartdoc/compare/7e171fc..7a9df65):
  7a9df65f  2024-02-20  Parker Lougheed  Add fallback text for sidebar failing to load (dart-lang/dartdoc#3643)
  9bcabb50  2024-02-20  Parker Lougheed  Fix missing left sidebar on extension type pages (dart-lang/dartdoc#3662)
  e8b8faa2  2024-02-16  Sam Rawlins  Include extension types in 'implementers' list (dart-lang/dartdoc#3658)

http (https://github.com/dart-lang/http/compare/6d9f9ef..ce0de37):
  ce0de37  2024-02-21  Derek Xu  Populate package:http_profile (dart-lang/http#1046)
  75e01f4  2024-02-20  Brian Quinlan  Create a simple WebSocket interface (dart-lang/http#1128)

markdown (https://github.com/dart-lang/markdown/compare/c2b8429..d735b0b):
  d735b0b  2024-02-21  Tom Yeh  Fix `#578`: list with checkbox mixed with empty lines (dart-lang/markdown#583)
  6efe141  2024-02-14  Kevin Moore  Migrate example to pkg:web, update minimum required Dart version (dart-lang/markdown#582)

protobuf (https://github.com/dart-lang/protobuf/compare/a293fb9..f085bfd):
  f085bfd  2024-02-20  Ömer Sinan Ağacan  Fix message_set.dart copyright year (google/protobuf.dart#912)

sse (https://github.com/dart-lang/sse/compare/af7d8d0..13ec752):
  13ec752  2024-02-20  Kevin Moore  blast_repo fixes (dart-lang/sse#104)
  2830dc9  2024-02-16  Kevin Moore  Support the latest pkg:web, require Dart 3.3 (dart-lang/sse#103)

stream_channel (https://github.com/dart-lang/stream_channel/compare/851336f..e02a5dd):
  e02a5dd  2024-02-16  Kevin Moore  Require Dart 3.3, update and fix lints (dart-lang/stream_channel#100)
  e62706e  2024-02-16  Kevin Moore  blast_repo fixes (dart-lang/stream_channel#101)

tools (https://github.com/dart-lang/tools/compare/2ef7673..9f4e6a4):
  9f4e6a4  2024-02-16  Elias Yishak  Helper to resolve dart version for clients of analytics (dart-lang/tools#233)
  8323b21  2024-02-13  Elias Yishak  New event added for sending analytics within package on errors (dart-lang/tools#229)

vector_math (https://github.com/google/vector_math.dart/compare/cb976c7..3706feb):
  3706feb  2024-02-18  dependabot[bot]  Bump dart-lang/setup-dart from 1.6.0 to 1.6.2 (google/vector_math.dart#313)

web (https://github.com/dart-lang/web/compare/a54a1f0..975e55c):
  975e55c  2024-02-15  Kevin Moore  Add TrustedTypes (dart-lang/web#173)
  0447807  2024-02-15  Srujan Gaddam  Add info on generation conventions (dart-lang/web#171)

webdev (https://github.com/dart-lang/webdev/compare/629c632..51b5484):
  51b54843  2024-02-14  Elliott Brooks  Implement `setFlag` for 'pause_isolates_on_start' (dart-lang/webdev#2373)

yaml_edit (https://github.com/dart-lang/yaml_edit/compare/2a9a11b..82ab64d):
  82ab64d  2024-02-21  Danny Tuppeny  Fix line endings for inserted maps on Windows (dart-lang/yaml_edit#66)
  6906ac4  2024-02-20  Devon Carew  update the publish workflow (dart-lang/yaml_edit#67)

Change-Id: I246c393586e3d6239925ac3cf3a6a245d86a2bf5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/353581
Reviewed-by: Kevin Moore <kevmoo@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
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.

2 participants