-
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 f34ef73
☁️ Nx Cloud last updated this comment at |
@code-pushup/ci
@code-pushup/cli
@code-pushup/core
@code-pushup/create-cli
@code-pushup/nx-plugin
@code-pushup/axe-plugin
@code-pushup/models
@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 d5f2a33 with previous commit 270b474. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories👍 4 groups improved, 👎 1 group regressed, 👍 11 audits improved, 👎 2 audits regressed, 23 audits changed without impacting score🗃️ Groups
29 other groups are unchanged. 🛡️ Audits
643 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 f523c36 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?
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.
Yes also thought about it. For me I'm pretty soled on going sync in only this edge case of WAL and profiling. Let's wait on @matejchalk opinion here.
| * @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()?
Co-authored-by: Hanna Skryl <80118140+hanna-skryl@users.noreply.github.com>
Co-authored-by: Hanna Skryl <80118140+hanna-skryl@users.noreply.github.com>
Co-authored-by: Hanna Skryl <80118140+hanna-skryl@users.noreply.github.com>
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