♻️ clean up createBatch API before migrating to js-core#4802
♻️ clean up createBatch API before migrating to js-core#4802BenoitZugmeyer wants to merge 8 commits into
createBatch API before migrating to js-core#4802Conversation
BenoitZugmeyer
changed the title
flushControllercreateBatch API before migrating to js-core
Bundles Sizes Evolution
|
|
BenoitZugmeyer
force-pushed
the
benoit/internalize-flush-controller
branch
2 times, most recently
from
1ddd0ad to
4fc2a8e
Compare
…ntityEncoder` Callers that don't need a custom encoder (telemetry, logs, debugger) no longer have to explicitly pass `createIdentityEncoder()`. Only callers with a non-default encoder (e.g. deflate in RUM) need to specify it.
`createBatch` now creates the `HttpRequest` internally, which removes boilerplate from every call site. The internal call is wrapped with `mockable()` so the spec can still inject a transport mock via `replaceMockable`.
The event payload also changes from `PageMayExitEvent` (`{ reason: PageExitReason }`)
to a new `UrgentFlushReason` type alias (= `PageExitReason`), so subscribers
receive the reason directly rather than a wrapped object.
…ceFlush` method `sessionExpireObservable` was the only "external trigger" wired directly into the flush controller, making it an awkward special case. Replacing it with a generic `forceFlush(reason)` method (re-exposed on `Batch`) lets callers decide when to flush and with what reason, keeping the flush controller focused on its internal triggers (page exit, bytes/messages/duration limits).
BenoitZugmeyer
force-pushed
the
benoit/internalize-flush-controller
branch
from
4fc2a8e to
ccc7356
Compare
|
/to-staging |
|
View all feedbacks in Devflow UI.
Commit f1e50ae278 will soon be integrated into staging-25.
We couldn't automatically merge the commit f1e50ae278 into staging-25! To solve the conflicts directly in Github, click here to create a fix pull request. Alternatively, you can also click here reset the integration branch or use the following Slack command: |
BenoitZugmeyer
force-pushed
the
benoit/internalize-flush-controller
branch
from
f1e50ae to
63e39dc
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1e50ae278
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| endpoints, | ||
| reportError, | ||
| }) | ||
| sessionExpireObservable.subscribe(() => batch.forceFlush('session_expire')) |
There was a problem hiding this comment.
When RUM is stopped or reinitialized, batch.stop() does not tear down this raw sessionExpireObservable subscription; the old callback keeps the stopped batch, encoder, and request state reachable and continues to run on future session expirations. Before this refactor the flush controller owned the session-expiry subscription and included it in its cleanup, so store the subscription here and include it in the returned stop path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is fine, we don't stop rum
| }), | ||
| }) | ||
| const batch = createBatch({ endpoints, reportError }) | ||
| sessionManager.expireObservable.subscribe(() => batch.forceFlush('session_expire')) |
There was a problem hiding this comment.
When Logs is stopped or reinitialized, batch.stop() only removes the batch flush observer and leaves this sessionManager.expireObservable subscription registered. Each stopped instance therefore keeps its batch/request state alive and still executes on later session expirations; keep the subscription and unsubscribe it from the returned stop path alongside batch.stop().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is fine, we don't stop rum
bdibon
left a comment
bdibon
left a comment
There was a problem hiding this comment.
It improves the whole batch / flush flow, very good!
3 participants
Motivation
In preparation for moving
createBatchtojs-core, this PR reduces thenumber of abstractions it leaks to callers. Previously, every entry-point
(
startLogsBatch,startRumBatch,startTelemetryTransport, …) had tomanually wire up
HttpRequest,FlushController, andPageMayExitObservable— details that are really internal to the batch machinery. Internalizing them
makes
createBatcha self-contained, easy-to-use primitive, and reduces thesurface that js-core would need to expose.
Changes
encoderoptional, defaulting tocreateIdentityEncoderHttpRequestcreation (callers now passendpoints+reportError)reportErrorto accept a plain message string instead of aRawError, so we don't have to exposeRawError(yet)PAGE_MAY_EXITlifecycle event toPREPARE_URGENT_FLUSHto better reflect its semantics and make it slightly more runtime agnosticsessionExpireObservableonFlushControllerwith aforceFlush(reason)method, so callers drive flush timing without being wired into the controllerFlushControllerandPageMayExitObservable;Batchnow exposesforceFlush,flushObservable, andprepareUrgentFlushObservabledirectlyTest instructions
This is a pure refactoring — verify that the SDK initialises and sends events normally by loading the sandbox (
yarn dev→http://localhost:8080) and checking that RUM and log events are dispatched as expected.Checklist