Conversation
🦋 Changeset detectedLatest commit: a2c5f8b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Code Review
This pull request updates the messaging service to initiate the logging service immediately upon enabling delivery metrics or staging a log, while also reducing the initial flush delay to zero. Feedback highlights a logic issue where starting the service with an empty queue could cause a 24-hour delay for subsequent events. Additionally, there is a risk of data loss during asynchronous log dispatching and a recommendation to clear the log queue when metrics export is disabled to ensure data privacy and memory efficiency.
Changeset File Check ✅
|
…n disabling BigQuery export
Doris-Ge
left a comment
There was a problem hiding this comment.
Is there any integration unit test we can add? For example, in the test, we set deliveryMetricsExportedToBigQueryEnabled to true, and then trigger onPush to verify that the logs are sent to FireLog. This test would help us catch the original bug that startLoggingService was not called anywhere.
added on in the sw test. |
Doris-Ge
left a comment
There was a problem hiding this comment.
I can't find a way to mark a resolved comment as unresolved. But in case you didn't see it, I posted another comment under this thread:
#9916 (comment)
Looks like the link does not work as I expected. I meant this https://screenshot.googleplex.com/8au9ubxMhUaaAvg Critique is definitely much better... |
agreed. good catch. forgot to paste it after polishing with ai. added in the sw listener file where it is most approparte as the entrance point for looging |
hsubox76
left a comment
There was a problem hiding this comment.
LG - is there a reported Github issue we can link users to in the release notes, to show what problem has been fixed, or was the bug discovered elsewhere?
Intent
Ship a fix so FCM Web delivery metrics / Firelog logging actually runs after developers enable
experimentalSetDeliveryMetricsExportedToBigQueryEnabled. TodaystageLogonly queues events in memory;startLoggingServicewas never started from production code, so_processQueue→_dispatchLogEventsnever fired and no POST toplay.google.com/logoccurred. This PR wiresstartLoggingServicefromstageLogand when enabling export, uses an immediate first flush (INITIAL_LOG_FLUSH_DELAY_MS = 0) for easier verification while keeping subsequent batches onLOG_INTERVAL_IN_MSafter dispatch.Summary
stageLog: callstartLoggingService(messaging)after enqueueing (idempotent)._setDeliveryMetricsExportedToBigQueryEnabled(..., true): callstartLoggingServiceso the flush loop arms when users opt in.startLoggingService: first_processQueuedelay0;_dispatchLogEventsstill reschedules withLOG_INTERVAL_IN_MS(24h).Root cause
startLoggingServicehad zero production call sites; onlystageLogappended tologEvents, so Firelogfetchnever ran.b/503660323