-
Notifications
You must be signed in to change notification settings - Fork 15
feat(utils): add WriteAheadLog classes #1210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
View your CI Pipeline Execution ↗ for commit a3641d3
☁️ 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: |
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared current commit f5a0e21 with previous commit 270b474. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories👍 2 groups improved, 👍 12 audits improved, 👎 1 audit regressed, 14 audits changed without impacting score🗃️ Groups
32 other groups are unchanged. 🛡️ Audits
652 other audits are unchanged. |
# Conflicts: # packages/utils/src/lib/sink-source.types.ts
# Conflicts: # testing/test-utils/src/lib/utils/perf-hooks.mock.ts
Code PushUp🥳 Code PushUp report has improved – compared current commit f5a0e21 with previous commit 270b474. 💼 Project
|
| 🏷️ Category | ⭐ Previous score | ⭐ Current score | 🔄 Score change |
|---|---|---|---|
| Documentation | 🔴 45 | 🟡 51 | |
| Code coverage | 🟢 95 | 🟢 95 |
4 other categories are unchanged.
👍 2 groups improved, 👍 9 audits improved
🗃️ Groups
| 🔌 Plugin | 🗃️ Group | ⭐ Previous score | ⭐ Current score | 🔄 Score change |
|---|---|---|---|---|
| JSDocs coverage | Documentation coverage | 🔴 45 | 🟡 51 | |
| Code coverage | Code coverage metrics | 🟢 95 | 🟢 95 |
13 other groups are unchanged.
🛡️ Audits
| 🔌 Plugin | 🛡️ Audit | 📏 Previous value | 📏 Current value | 🔄 Value change |
|---|---|---|---|---|
| JSDocs coverage | Classes coverage | 🟨 3 undocumented classes | 🟨 3 undocumented classes | |
| JSDocs coverage | Properties coverage | 🟥 27 undocumented properties | 🟥 37 undocumented properties | |
| JSDocs coverage | Types coverage | 🟥 66 undocumented types | 🟨 56 undocumented types | |
| JSDocs coverage | Functions coverage | 🟥 239 undocumented functions | 🟥 242 undocumented functions | |
| JSDocs coverage | Variables coverage | 🟥 49 undocumented variables | 🟥 57 undocumented variables | |
| JSDocs coverage | Methods coverage | 🟨 13 undocumented methods | 🟨 15 undocumented methods | |
| Code coverage | Function coverage | 🟩 96.2 % | 🟩 96.6 % | |
| Code coverage | Branch coverage | 🟩 91.8 % | 🟩 92.2 % | |
| Code coverage | Line coverage | 🟩 97.8 % | 🟩 98 % |
435 other audits are unchanged.
13 other projects are unchanged.
hanna-skryl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The justification for sync file operations is well-reasoned 👍 I've added a few minor suggestions and comments.
| SHARD_ID: /^\d{8}-\d{6}-\d{3}(?:\.\d+){3}$/, | ||
| } as const; | ||
|
|
||
| export function soratebleReadableDateString(timestampMs: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| export function soratebleReadableDateString(timestampMs: string): string { | |
| export function sortableReadableDateString(timestampMs: string): string { |
| * @throws Error if recovery encounters decode errors | ||
| */ | ||
| repack(out = this.#file) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSDoc says that repack() throws if recovery encounters decode errors, but the implementation logs to the console and continues without throwing. Am I missing something?
| import type { Codec } from '../src/lib/wal.js'; | ||
|
|
||
| export class MockSink implements Sink<string, string> { | ||
| export class MockFileSink implements WriteAheadLogFile<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TypeScript plugin reports an error here. Maybe MockFileSink should implement AppendableSink<string> instead?
| it('should create WAL format with custom directory', () => { | ||
| const format = traceEventWalFormat({ dir: '/custom/path' }); | ||
|
|
||
| expect(format.baseName).toBe('trace'); | ||
| expect(format.walExtension).toBe('.jsonl'); | ||
| expect(format.finalExtension).toBe('.json'); | ||
| }); | ||
|
|
||
| it('should create WAL format with groupId', () => { | ||
| const format = traceEventWalFormat({ groupId: 'session-123' }); | ||
|
|
||
| expect(format.baseName).toBe('trace'); | ||
| expect(format.walExtension).toBe('.jsonl'); | ||
| expect(format.finalExtension).toBe('.json'); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
traceEventWalFormat doesn't take any parameters 🤔
| const recordsToWrite = hasInvalidEntries | ||
| ? (r.records as T[]) | ||
| : filterValidRecords(r.records); | ||
| fs.mkdirSync(path.dirname(out), { recursive: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an existing ensureDirectoryExists utility, but it's async. Maybe worth adding a sync variant (ensureDirectoryExistsSync) since this pattern repeats multiple times?
| * @throws Error if any shard contains decode errors | ||
| */ | ||
| finalize(opt?: Record<string, unknown>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as in repack() - the JSDoc says it throws on decode errors, but there is no explicit Error.
| format: Partial<WalFormat<T>>, | ||
| ): WalFormat<T> { | ||
| const { | ||
| baseName = 'trace', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a mismatch between the JSDoc and the actual implementation - should baseName default to 'trace' or Date.now().toString()?
| /** | ||
| * Initialize the origin PID environment variable if not already set. | ||
| * This must be done as early as possible before any user code runs. | ||
| * Set's envVarName to the current process PID if not already defined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Set's envVarName to the current process PID if not already defined. | |
| * Sets envVarName to the current process ID if not already defined. |
| /** | ||
| * Pure helper function to recover records from WAL file content. | ||
| * @param content - Raw file content as string | ||
| * @param decode - function fo decoding records |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @param decode - function fo decoding records | |
| * @param decode - function for decoding records |
| * @param dir - The directory to store the shard file | ||
| * @param format - The WalFormat to use for the shard file | ||
| * @param groupId - The human-readable group ID (yyyymmdd-hhmmss-ms format) | ||
| * @param shardId - The human-readable shard ID (readable-timestamp.pid.threadId.count format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @param dir - The directory to store the shard file | |
| * @param format - The WalFormat to use for the shard file | |
| * @param groupId - The human-readable group ID (yyyymmdd-hhmmss-ms format) | |
| * @param shardId - The human-readable shard ID (readable-timestamp.pid.threadId.count format) | |
| * @param opt.dir - The directory to store the shard file | |
| * @param opt.format - The WalFormat to use for the shard file | |
| * @param opt.groupId - The human-readable group ID (yyyymmdd-hhmmss-ms format) | |
| * @param opt.shardId - The human-readable shard ID (readable-timestamp.pid.threadId.count format) |
Related: #1197
This PR includes:
wal.ts- general WriteAheadLog and sharded WAL inc format and filenameswal-trace-json.tstraceEvent specific login for a WALNote
This PR addresses a comment related to logging.
It could also be used as file output for the logger
Warning
All filesystem operations in WAL—and indirectly in the profiler—are synchronous by design.
The
eslintrules are disables over file name patternWhy synchronous
fsis used for WAL code?process.on('exit')cannot safely run async I/O