feat(logging): enable log rotation and set retry on full log store sync #3699

NikaHsn · 2023-09-06T00:17:29Z

Issue #, if available:

Description of changes:

make QueuedItemStore.isFull() sync
enable log rotation when log store is full
set retry on full log store sync
handle rejectedLogEventsInfo response and do partial sync

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

dnys1 · 2023-09-06T18:14:42Z

...udwatch/amplify_logging_cloudwatch/lib/src/queued_item_store/dart_queued_item_store.web.dart

+    final db = _database;
    await db.deleteItems(items);


Here and throughout

Suggested change

final db = _database;

await db.deleteItems(items);

await _database.deleteItems(items);

dnys1 · 2023-09-06T18:27:00Z

...dwatch/amplify_logging_cloudwatch/lib/src/queued_item_store/index_db/indexed_db_adapter.dart

-      final openRequest = indexedDB!.open('test', 1);
-      await openRequest.future;
+      indexedDB!.open('test', 1).result;


Can you explain this change?

QueuedItemStore.isFull() is a sync method. however it was implemented async because the web implementation calls checkIsIndexedDBSupported to either use indexedDB or InMemoryQueuedItemStore. Because the checkIsIndexedDBSupported was async all the web APIs had to be async.

But why would this be guaranteed to throw in the same way as before?

based on the docs If the operation is successful, the value of result is a connection to the database. If the request failed and the result is not available, an InvalidStateError exception is thrown.

dnys1 · 2023-09-06T18:27:51Z

packages/logging_cloudwatch/aws_logging_cloudwatch/lib/src/cloudwatch_logger_plugin.dart

+            logs.removeRange(0, expiredLogEventEndIndex + 1);
+            // set events to start from next
+            events.removeRange(0, expiredLogEventEndIndex + 1);
+            continue;


Why can't multiple of these blocks apply?

dnys1 · 2023-09-06T18:28:44Z

packages/logging_cloudwatch/aws_logging_cloudwatch/lib/src/cloudwatch_logger_plugin.dart

+          final tooNewLogEventStartIndex =
+              rejectedLogEventsInfo?.tooNewLogEventStartIndex;
+
+          if (expiredLogEventEndIndex != null &&


Can you extract this logic into a function so it's not duplicated 3 times?

dnys1 · 2023-09-07T21:07:34Z

packages/logging_cloudwatch/aws_logging_cloudwatch/lib/src/cloudwatch_logger_plugin.dart

+          if (tooNewLogEventStartIndex != null &&
+              tooNewLogEventStartIndex >= 0 &&
+              tooNewLogEventStartIndex <= events.length - 1) {
+            tooNewException = _TooNewLogEventException(


Why not throw it here? The control flow would be easier to follow

we want to continue with syncing the current batch after sanitizing the events in the current batch and then throw to stop syncing next batches

dnys1 · 2023-09-07T21:08:54Z

packages/logging_cloudwatch/aws_logging_cloudwatch/lib/src/cloudwatch_logger_plugin.dart

        _syncing = false;
      }
    }
  }

+  void _handleFullLogStoreAfterSync({
+    int retryTimeInMillisecondsSinceEpoch = 0,


Can you change this logic to work with DateTime and Duration objects instead of integers? This is very opaque and hard to follow

dnys1 · 2023-09-07T21:10:54Z

packages/logging_cloudwatch/aws_logging_cloudwatch/lib/src/cloudwatch_logger_plugin.dart

+            logs.removeRange(tooNewLogEventStartIndex, logs.length);
+            // set events to end at the index
+            events.removeRange(tooNewLogEventStartIndex, events.length);
+            continue;


Why continue if all batches after this will be newer?

we want to continue with syncing the current batch after sanitizing the events in the current batch and then throw to stop syncing next batches

dnys1 · 2023-09-11T18:29:08Z

...dwatch/amplify_logging_cloudwatch/lib/src/queued_item_store/index_db/indexed_db_adapter.dart

-      final openRequest = indexedDB!.open('test', 1);
-      await openRequest.future;
+      indexedDB!.open('test', 1).result;


But why would this be guaranteed to throw in the same way as before?

dnys1 · 2023-09-11T18:31:39Z

packages/logging_cloudwatch/aws_logging_cloudwatch/lib/src/cloudwatch_logger_plugin.dart

+    int? pastEndIndex;
+    int? futureStartIndex;
+
+    if (_isValidIndex(tooOldLogEventEndIndex, length)) {


Why is this allowed to be false? I would use RangeError.checkValidIndex to assert these are valid.

dnys1 · 2023-09-11T18:33:20Z

packages/logging_cloudwatch/aws_logging_cloudwatch/lib/src/cloudwatch_logger_plugin.dart

+            final (tooOldEndIndex, tooNewStartIndex) =
+                rejectedLogEventsInfo.parse(events.length);
+
+            if (_isValidIndex(tooNewStartIndex, events.length)) {


You've already checked this

RejectedLogEventsInfo.parse() validates RejectedLogEventsInfo indices.
here is validating the indices returned by parse() method.

dnys1 · 2023-09-11T18:34:13Z

packages/logging_cloudwatch/aws_logging_cloudwatch/lib/src/cloudwatch_logger_plugin.dart

+            await _logStore.deleteItems(logs);
+            break;
+          }
+          {


Remove these braces; they're unnecessary

dnys1 · 2023-09-11T18:34:21Z

packages/logging_cloudwatch/aws_logging_cloudwatch/lib/src/cloudwatch_logger_plugin.dart

+              // set events to start after the index.
+              events.removeRange(0, tooOldEndIndex + 1);
+            }
+            continue;


Can remove continue now

dnys1 · 2023-09-11T18:40:22Z

packages/logging_cloudwatch/aws_logging_cloudwatch/lib/src/cloudwatch_logger_plugin.dart

+        _logStore.isFull(_pluginConfig.localStoreMaxSizeInMB);
+    if (!isLogStoreFull) {
+      _retryCount = 0;
+      _retryTime = null;


Why would you not respect retryTime here?

it is reseting retry if log store is not full. it retries to sync only if log store is full otherwise can wait till next sync.

dnys1 · 2023-09-11T18:41:01Z

packages/logging_cloudwatch/aws_logging_cloudwatch/lib/src/cloudwatch_logger_plugin.dart

+      return;
+    }
+    _retryCount += 1;
+    _retryTime = DateTime.timestamp().add((_baseRetryInterval * _retryCount));


This seems like a long time for a basic backoff

flushIntervalInSeconds = const Duration(seconds: 60). updated the _baseRetryInterval to 10 secs so there will be 2-3 retries within the flush interval.

dnys1 · 2023-09-11T18:42:14Z

packages/logging_cloudwatch/aws_logging_cloudwatch/lib/src/cloudwatch_logger_plugin.dart

+        // after sending each batch to CloudWatch check if the batch has
+        // `tooNewException` and throw to stop syncing next batches.
+        if (tooNewException != null) {
+          throw tooNewException;


I think this would be cleaner if you just returned the retry time instead of throwing

…nc (#3699) * feat(logging): enable log rotation and set retry

feat(logging): enable log rotation and set retry

ddd57a8

NikaHsn requested a review from a team as a code owner September 6, 2023 00:17

khatruong2009 approved these changes Sep 6, 2023

View reviewed changes

dnys1 reviewed Sep 7, 2023

View reviewed changes

Nika Hassani added 2 commits September 8, 2023 15:18

address review comments

e6fd967

use datetime and duration instead of int miliseconds since epoch

c4eb744

dnys1 reviewed Sep 11, 2023

View reviewed changes

address review comments

8e2aa13

dnys1 approved these changes Sep 12, 2023

View reviewed changes

NikaHsn merged commit eda0a6a into aws-amplify:feat/logging/cloudwatch Sep 12, 2023
21 checks passed

NikaHsn deleted the feat/logging/enable-log-rotation branch October 13, 2023 21:34

khatruong2009 pushed a commit that referenced this pull request Nov 27, 2023

feat(logging): enable log rotation and set retry on full log store sy…

5751957

…nc (#3699) * feat(logging): enable log rotation and set retry

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(logging): enable log rotation and set retry on full log store sync #3699

feat(logging): enable log rotation and set retry on full log store sync #3699

NikaHsn commented Sep 6, 2023

dnys1 Sep 6, 2023

dnys1 Sep 6, 2023

NikaHsn Sep 8, 2023

dnys1 Sep 11, 2023

NikaHsn Sep 11, 2023

dnys1 Sep 6, 2023

dnys1 Sep 6, 2023

dnys1 Sep 7, 2023

NikaHsn Sep 8, 2023

dnys1 Sep 7, 2023

dnys1 Sep 7, 2023

NikaHsn Sep 8, 2023

dnys1 Sep 11, 2023

dnys1 Sep 11, 2023

dnys1 Sep 11, 2023

NikaHsn Sep 12, 2023

dnys1 Sep 11, 2023

dnys1 Sep 11, 2023

dnys1 Sep 11, 2023

NikaHsn Sep 11, 2023

dnys1 Sep 11, 2023

NikaHsn Sep 11, 2023

dnys1 Sep 11, 2023

	final db = _database;
	await db.deleteItems(items);
	await _database.deleteItems(items);

feat(logging): enable log rotation and set retry on full log store sync #3699

feat(logging): enable log rotation and set retry on full log store sync #3699

Conversation

NikaHsn commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment