feat(browser): opt-in offline queue for spans and logs - #1156
Conversation
MapleBrowser.sendFeedback({ message, email?, name?, attributes? }) sends
a maple.user_feedback OTel log event: session.id, user.email / user.name
(semconv; the email is dropped when privacy.captureUserEmail is false),
maple.feedback.has_replay and, when the user hit an error earlier in the
page, maple.feedback.error_trace_id. The event is linked to that error's
span, so it shows on the error's trace.
Feedback also keeps a buffered replay, the same way an error does: a user
reporting a problem is as good a signal. Headless for now; a drop-in
widget can come as its own entry so it never touches the eager bundle.
Eager budget 43 -> 43.5 kB, first-party 17.5 -> 18 kB.
The OTLP exporters retry a failed export for about 10 seconds, then the batch is gone. transport.offline keeps it instead. - A thin eager wrapper sits right around the OTLP trace exporter (after the consent and HTTP status policies, so what is kept is what would have been sent) and hands failed batches to the deferred chunk, holding up to 20 until it lands. The logs exporter is wrapped the same way. - The deferred chunk serializes them with the OTLP JSON serializers the exporters use and stores them in IndexedDB, then POSTs the raw payloads again on `online` and on the next page load. A 5xx or 429 keeps the rest for later; any other response drops the batch. At most 100 batches, 24h. - Revoking consent clears the queue. No IndexedDB (private windows): nothing is kept. Off by default. otlp-transformer becomes a direct dependency (it was already bundled through the exporters).
|
Warning Review limit reachedNext included review available in 55 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Maple reviewConfidence 2/5 · risky as written Adds an opt-in (
FindingsWarning · F1 ·
|
| Change | Kind | Observable | Evidence |
|---|---|---|---|
| resend POST to /v1/traces|logs from IndexedDB | outbound | no | FetchInstrumentation ignores ${config.endpoint}/v1/ (packages/browser/src/tracing.ts:240), same as the exporters' own ingest calls, so no span is expected |
| OfflineSpanExporter / OfflineLogExporter wrapper | span/log export path | yes | keeps the HttpStatus/Consent exporter chain and the existing BatchSpanProcessor, no new span surface |
Copy all findings (1)
Findings from an automated review of commit 374a42264bac36e7b263339ef72168775048cfd4. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.
---
F1 · Warning · correctness · packages/browser/src/deferred/offline.ts:89-96
`resend` re-uploads batches captured before the current consent grant
`resend` only checks `hasConsent()` and `MAX_AGE_MS`, so it sends batches whose `createdAt` predates the current grant. Consent captures are cleared on `onConsentChange`, but that only runs while the queue is live: a revoke that lands after `stop()` (or a queue closed when consent flipped) leaves the rows in IndexedDB, and the next page load — where `consentAllowedSince()` is the moment of the new grant — posts them. That is the case `consent.ts` documents as "discard anything buffered before a late grant or across a revoke/re-grant cycle", and which `ConsentSpanExporter`/`ConsentLogExporter` already enforce per record (`packages/browser/src/tracing.ts:16`).
Suggested fix: Treat a batch older than the current grant like an expired one: add `consentAllowedSince` to the `@maple/browser-session` import and inside the loop require `batch.createdAt >= consentAllowedSince()` before sending, deleting the batch otherwise.
374a422 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple to ask about one.
![maple-review-bot[bot]](https://avatars.githubusercontent.com/in/4108169?s=60&v=4){kind=small}
maple.feedback.has_replay was read before keepReplay() ran, so on the buffered-replay path it was always false for the very feedback that kept the replay. keepReplay() now runs first; the trigger marks the session recorded synchronously.
A revoke the live queue never saw (it landed after stop(), or on another page) left batches in IndexedDB, and the next page load sent them under its fresh grant. resend now drops batches older than consentAllowedSince(), the rule the consent exporters already apply per record.
|
Note A newer push replaced |
lastError survived shutdown(), so feedback after a re-init linked to a trace from the previous lifecycle. configureFeedback, run by init() and shutdown(), now clears it.
Maple reviewConfidence 3/5 · needs attention Adds an opt-in, IndexedDB-backed offline queue: span and log batches the exporters gave up on are stored as OTLP JSON and POSTed again on
FindingsWarning · F2 ·
|
sendFeedback was a thin add-on with nowhere in the product to read it. End-user feedback should come back as a proper feature built on product events, not as a log event with no surface. Size budgets are re-attributed to the offline queue's exporter wrapper, which used the headroom.
Maple reviewConfidence 3/5 · needs attention The head commit drops the headless
Still open from earlier reviews
What was checked
Observability coverage: 1 of 1 changes observable
|
|
Note A newer push replaced |
IndexedDB is shared by every tab of an origin, so two tabs resending on `online` or page load both read and POSTed the same batches. Resends now run under a Web Locks lease, so tabs take turns and a later one finds the sent batches already deleted. A resend called while one is in flight now joins it instead of returning before the work is done.
Maple reviewConfidence 3/5 · needs attention Adds an opt-in (
FindingsWarning · F3 · The resend lease is held across POSTs that have no timeoutcorrectness ·
What was checked
Copy all findings (1)
|
| try { | ||
| // The store is shared by every tab of the origin: tabs drain it in turn, and a | ||
| // later one finds what an earlier one sent already deleted. | ||
| if (typeof navigator !== "undefined" && navigator.locks) { |
There was a problem hiding this comment.
The resend lease is held across POSTs that have no timeout
F3 · Warning · correctness
run takes the origin-wide navigator.locks lease and holds it for the whole drain, whose fetch (line 93) has no timeout. One tab with a stalled connection (the flaky-network case this feature exists for) keeps the lease, so no other tab of the origin can resend, and every online event in that tab joins the same inflight drain (line 124) instead of starting a new one — the stored batches then wait for the next page load. Abort the POST after a short budget (AbortSignal.timeout), which the existing .catch(() => undefined) already turns into "keep it for next time".
Add `signal: AbortSignal.timeout(10_000)` to the drain `fetch`, or scope the lock to the store read/delete and not to the network call.
Prompt for an AI agent
In `packages/browser/src/deferred/offline.ts:112-116`: The resend lease is held across POSTs that have no timeout.
`run` takes the origin-wide `navigator.locks` lease and holds it for the whole `drain`, whose `fetch` (line 93) has no timeout. One tab with a stalled connection (the flaky-network case this feature exists for) keeps the lease, so no other tab of the origin can resend, and every `online` event in that tab joins the same `inflight` drain (line 124) instead of starting a new one — the stored batches then wait for the next page load. Abort the POST after a short budget (`AbortSignal.timeout`), which the existing `.catch(() => undefined)` already turns into "keep it for next time".
Suggested fix: Add `signal: AbortSignal.timeout(10_000)` to the drain `fetch`, or scope the lock to the store read/delete and not to the network call.
Verify the problem exists at that location before changing it, and keep the fix to those lines.
1 participant
Part 10 of the browser SDK stack. Based on #1154 (the feedback PR #1155 was dropped; this PR removes its code again, so the net diff against #1154 is the offline queue only).
The OTLP exporters retry a failed export for about 10 seconds (
retrying-transport, 5 attempts within the export timeout), then the batch is gone.transport.offlinekeeps it instead.What changes
@opentelemetry/otlp-transformer, now a direct dependency; it was already bundled) and stores them in IndexedDB.onlineand on the next page load. A 5xx or 429 keeps the rest for later; any other response drops the batch. At most 100 batches, for up to 24h.Testing
Browser tests against real IndexedDB:
fetchfails, then resent as OTLP JSON to/v1/tracesand removed once back online;Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.