refactor: use sharded WAL #1231
Open
+4,884
−3,448
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
📖 Project documentation
|
View your CI Pipeline Execution ↗ for commit e03a7ac
☁️ Nx Cloud last updated this comment at |
@code-pushup/ci
@code-pushup/cli
@code-pushup/core
@code-pushup/create-cli
@code-pushup/models
@code-pushup/nx-plugin
@code-pushup/axe-plugin
@code-pushup/coverage-plugin
@code-pushup/eslint-plugin
@code-pushup/js-packages-plugin
@code-pushup/jsdocs-plugin
@code-pushup/lighthouse-plugin
@code-pushup/typescript-plugin
@code-pushup/utils
commit: |
# Conflicts: # packages/utils/docs/profiler.md # packages/utils/src/lib/profiler/constants.ts # packages/utils/src/lib/profiler/profiler.int.test.ts # packages/utils/src/lib/profiler/profiler.ts # packages/utils/src/lib/profiler/profiler.unit.test.ts
Contributor
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared current commit f4bebc5 with previous commit 7b6e72c. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories
👍 1 group improved, 👎 4 groups regressed, 👍 5 audits improved, 👎 10 audits regressed, 15 audits changed without impacting score🗃️ Groups
29 other groups are unchanged. 🛡️ Audits
649 other audits are unchanged. |
Contributor
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared current commit f4bebc5 with previous commit 7b6e72c. 💼 Project
|
| 🏷️ Category | ⭐ Previous score | ⭐ Current score | 🔄 Score change |
|---|---|---|---|
| Documentation | 🟡 61 | 🟡 61 |  |
| Code coverage | 🟢 95 | 🟢 94 |  |
4 other categories are unchanged.
👍 1 group improved, 👎 1 group regressed, 👍 3 audits improved, 👎 5 audits regressed
🗃️ Groups
| 🔌 Plugin | 🗃️ Group | ⭐ Previous score | ⭐ Current score | 🔄 Score change |
|---|---|---|---|---|
| JSDocs coverage | Documentation coverage | 🟡 61 | 🟡 61 | |
| Code coverage | Code coverage metrics | 🟢 95 | 🟢 94 | |
13 other groups are unchanged.
🛡️ Audits
| 🔌 Plugin | 🛡️ Audit | 📏 Previous value | 📏 Current value | 🔄 Value change |
|---|---|---|---|---|
| JSDocs coverage | Variables coverage | 🟥 50 undocumented variables | 🟥 50 undocumented variables |  |
| JSDocs coverage | Types coverage | 🟨 55 undocumented types | 🟥 60 undocumented types |  |
| JSDocs coverage | Methods coverage | 🟨 10 undocumented methods | 🟨 12 undocumented methods |  |
| JSDocs coverage | Functions coverage | 🟥 243 undocumented functions | 🟥 242 undocumented functions |  |
| JSDocs coverage | Properties coverage | 🟥 39 undocumented properties | 🟥 40 undocumented properties |  |
| Code coverage | Branch coverage | 🟩 91.9 % | 🟩 91.4 % |  |
| Code coverage | Function coverage | 🟩 95.8 % | 🟩 95.5 % |  |
| Code coverage | Line coverage | 🟩 97.8 % | 🟩 97.7 % |  |
436 other audits are unchanged.
13 other projects are unchanged.
There was a problem hiding this comment.
Pull request overview
Refactors NodeJS profiling persistence to use a sharded write-ahead log (WAL) flow and updates trace serialization/normalization utilities and tests accordingly.
Changes:
- Introduces
ShardedWal+ process/time ID helpers and wires NodeJS profiler persistence to shard/merge traces. - Refactors trace event encoding/decoding and trace file generation to support deterministic output.
- Moves trace JSON(L) normalization helpers from
@code-pushup/test-utilsintopackages/utils/mocksand updates Vitest include patterns.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| testing/test-utils/src/lib/utils/omit-trace-json.unit.test.ts | Removes old trace JSONL normalization unit tests from test-utils. |
| testing/test-utils/src/lib/utils/omit-trace-json.ts | Removes old trace JSONL normalization helper from test-utils. |
| testing/test-utils/src/index.ts | Stops exporting removed omit-trace-json helper. |
| testing/test-setup-config/src/lib/vitest-config-factory.unit.test.ts | Updates Vitest config factory tests to include mocks/** patterns. |
| testing/test-setup-config/src/lib/vitest-config-factory.ts | Adds mocks/** include globs for unit/int tests. |
| packages/utils/src/lib/wal.ts | Refactors WAL types/codec behavior; removes in-file sharded WAL implementation. |
| packages/utils/src/lib/wal.int.test.ts | Adds integration tests for WriteAheadLogFile. |
| packages/utils/src/lib/wal-sharded.unit.test.ts | Adds unit tests for new ShardedWal behavior (memfs). |
| packages/utils/src/lib/wal-sharded.ts | Introduces new ShardedWal implementation and shard ID generation. |
| packages/utils/src/lib/wal-sharded.int.test.ts | Adds integration tests for ShardedWal file behavior. |
| packages/utils/src/lib/user-timing-extensibility-api-utils.ts | Adjusts track config typing to omit dataType. |
| packages/utils/src/lib/profiler/wal-json-trace.unit.test.ts | Expands codec + trace format tests for new serialization behavior. |
| packages/utils/src/lib/profiler/wal-json-trace.ts | Updates trace WAL format to use new codec + trace file helpers and base name constant. |
| packages/utils/src/lib/profiler/trace-file.type.ts | Simplifies trace event typings and trace container typing. |
| packages/utils/src/lib/profiler/trace-file-utils.unit.test.ts | Updates tests to new trace event factories and encoding/decoding. |
| packages/utils/src/lib/profiler/trace-file-utils.ts | Refactors trace event factories and adds event encode/decode + (de)serialization helpers. |
| packages/utils/src/lib/profiler/profiler.unit.test.ts | Updates unit tests to reflect new enable-state API usage. |
| packages/utils/src/lib/profiler/profiler.ts | Adds debug state to profiler options and env var handling. |
| packages/utils/src/lib/profiler/profiler.int.test.ts | Updates integration tests for new profiler defaults and track payload expectations. |
| packages/utils/src/lib/profiler/profiler-node.ts | Switches NodeJS profiler persistence to ShardedWal and updates lifecycle/stats. |
| packages/utils/src/lib/profiler/profiler-node.int.test.ts | Reworks NodeJS profiler integration tests for sharded output, snapshots, multiprocess test. |
| packages/utils/src/lib/profiler/constants.ts | Adds sharding/output env vars and switches debug env var to DEBUG. |
| packages/utils/src/lib/profiler/snapshots/sharded-path-trace-events.jsonl | Removes snapshot for previous single-shard JSONL output. |
| packages/utils/src/lib/profiler/snapshots/entries-write-to-shard.jsonl | Adds snapshot for normalized shard JSONL output. |
| packages/utils/src/lib/profiler/snapshots/entries-write-to-shard.json | Adds snapshot for normalized finalized trace JSON output. |
| packages/utils/src/lib/profiler/snapshots/debugMode-test.json | Adds snapshot for debug-mode trace output. |
| packages/utils/src/lib/profiler/snapshots/custom-tracks-trace-events.jsonl | Removes snapshot for previous custom-tracks JSONL output. |
| packages/utils/src/lib/profiler/snapshots/comprehensive-stats-trace-events.jsonl | Removes snapshot for previous stats JSONL output. |
| packages/utils/src/lib/profiler/snapshots/buffered-test.json | Adds snapshot for buffered-entry capture output. |
| packages/utils/src/lib/process-id.unit.test.ts | Adds tests for new time/process/thread ID utilities. |
| packages/utils/src/lib/process-id.ts | Introduces helpers for unique time/process/thread/instance IDs. |
| packages/utils/src/lib/performance-observer.unit.test.ts | Updates tests for new debug env var and buffered behavior changes. |
| packages/utils/src/lib/performance-observer.ts | Refactors buffered capture strategy and standardizes debug env var usage. |
| packages/utils/src/lib/create-runner-files.ts | Uses new getUniqueProcessThreadId for per-runner uniqueness. |
| packages/utils/mocks/omit-trace-json.unit.test.ts | Adds tests for new trace normalization helpers in mocks. |
| packages/utils/mocks/omit-trace-json.ts | Adds new trace JSON/JSONL loading + normalization utilities for deterministic snapshots. |
| packages/utils/mocks/multiprocess-profiling/utils.ts | Adds multiprocess profiling helper utilities. |
| packages/utils/mocks/multiprocess-profiling/profiler-worker.mjs | Adds multiprocess coordinator script for profiling tests/demos. |
| packages/utils/mocks/multiprocess-profiling/profiler-worker-child.mjs | Adds multiprocess child worker script for profiling tests/demos. |
| packages/utils/mocks/README.md | Documents multiprocess profiling mocks usage. |
| packages/utils/eslint.config.js | Allows sync fs usage in WAL/profiler test files. |
| packages/utils/docs/profiler.md | Updates profiler docs for sharded WAL, env vars, state machine; includes new config tables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+28
to
+30
| const filePath = path.join(testDir, 'test.log'); | ||
| walFile = new WriteAheadLogFile({ file: filePath, codec: stringCodec() }); | ||
|
|
There was a problem hiding this comment.
WriteAheadLogFile<string> is constructed with stringCodec(), but stringCodec is now declared as Codec<T extends object>. This won’t type-check. Either adjust the codec typing/implementation to support strings or update the test (and any call sites) to use object records.
Comment on lines
+16
to
+17
| let shardedWal: ShardedWal<string>; | ||
|
|
There was a problem hiding this comment.
ShardedWal is declared as ShardedWal<T extends object>, but this test uses ShardedWal<string>, which won’t compile under the new constraint. Either loosen the generic constraint or update tests/usage to use object records and an appropriate codec.
Comment on lines
+112
to
+116
| format: profilerFormat, | ||
| measureName, | ||
| outDir = PROFILER_PERSIST_OUT_DIR, | ||
| enabled, | ||
| filename, | ||
| debugEnvVar = PROFILER_DEBUG_ENV_VAR, | ||
| debug, |
There was a problem hiding this comment.
enabled and debug are destructured here but never used, which will trip unused-variable linting and also means the NodejsProfilerOptions debug flag has no effect on profiler debug mode.
Comment on lines
+156
to
+159
| constructor(options: { id?: string; file: string; codec: Codec<T> }) { | ||
| const { file, codec } = options; | ||
| this.#file = file; | ||
| const c = createTolerantCodec(codec); |
There was a problem hiding this comment.
WriteAheadLogFile constructor now accepts an optional id, but the value is never stored or used. Either remove the parameter or persist/use it (e.g., for stats/logging) to avoid a misleading API.
Comment on lines
+283
to
290
| export const stringCodec = <T extends object = object>(): Codec<T> => ({ | ||
| encode: v => JSON.stringify(v), | ||
| decode: v => { | ||
| try { | ||
| return JSON.parse(v) as T; | ||
| } catch { | ||
| return v as T; | ||
| return v as unknown as T; | ||
| } |
There was a problem hiding this comment.
stringCodec is typed as Codec<T extends object> but the implementation still round-trips strings at runtime (via JSON.stringify/parse). This creates a type-level mismatch with usages like WriteAheadLogFile<string>({ codec: stringCodec() }). Either widen the generic constraint (e.g. T extends object | string) or provide a separate JSON-object codec and keep a true string passthrough codec.
| | `format` | `ProfilerFormat<DomainEvents>` | _required_ | WAL format configuration for sharded write-ahead logging, including `encodePerfEntry` | | ||
| | `measureName` | `string` | _auto-generated_ | Optional folder name for sharding. If not provided, a new group ID will be generated | | ||
| | `outDir` | `string` | `'tmp/profiles'` | Output directory for WAL shards and final files | | ||
| | `outBaseName` | `string` | _optional_ | Override the base name for WAL files (overrides format.baseName) | |
There was a problem hiding this comment.
The options table lists outBaseName, but there is no corresponding option in NodejsProfilerOptions/PersistOptions (and no code uses it). This is misleading API documentation; either implement the option or remove it from the docs.
Suggested change
| | `outBaseName` | `string` | _optional_ | Override the base name for WAL files (overrides format.baseName) | |
Comment on lines
+267
to
269
| export type WalFormat<T extends object> = { | ||
| /** Base name for the WAL (e.g., "trace") */ | ||
| baseName: string; |
There was a problem hiding this comment.
WalFormat is now constrained to T extends object, but the WAL implementation and tests in this PR still write primitive/string records. This type change will prevent using WalFormat<string> (and ShardedWal<string>) at compile time. Either restore T extends object | string or update all call sites/tests to use object records/codecs only.
Collaborator
Author
There was a problem hiding this comment.
@copilot If I add string | objects the typing gets complicated as every T has to extend it... is there a better more ergonomic option?
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related:
@cursor review