Improve internal instrumentation module#20119
Conversation
While working on another feature, I used this instrumentation and made some changes. So why not commit it? Added a `let value = I.span(label, () => 123)` callback version such that you don't need to use `I.start(label)` and `I.end(label)`. It also returns the value, but that's optional. This also handles async/promises. Additionally added `using _ = I.track(label)` which uses the new `using` keyword and it will internally call `I.end(label)` whenever the variable `_` goes out of scope. This is useful in situations where you don't want an additionaly wrapping function, or where it's easy to forget to call `I.end(label)` if there are mutliple exit points (return) of a function.
Confidence Score: 4/5Safe to merge; changes are confined to internal instrumentation helpers with no public API surface, and all existing callers are unaffected. The implementation logic for both new APIs is sound, and the constructor parameter addition is backward-compatible since all call sites use the no-argument form. The two concerns — a potentially flaky real-timer snapshot and undocumented async-span interleaving behavior — are low-impact for internal tooling but worth addressing before the PR lands. The async test in instrumentation.test.ts deserves a second look due to its hard-coded timing snapshot. Reviews (1): Last reviewed commit: "improve instrumentation" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it('should measure async callbacks via the `span` api', async () => { | ||
| let I = new Instrumentation() | ||
|
|
||
| await I.span('Foo', async () => { | ||
| let x = 1 | ||
| for (let i = 0; i < 100; i++) { | ||
| I.span('Bar', () => { | ||
| x **= 2 | ||
| }) | ||
| } | ||
| await new Promise((r) => setTimeout(r, 500)) | ||
| }) | ||
|
|
||
| expect.assertions(1) | ||
|
|
||
| I.report((output) => { | ||
| expect(stabilize(output)).toMatchInlineSnapshot(` | ||
| " | ||
| [510.00ms] Foo | ||
| [ 0.05ms] ↳ Bar × 100 | ||
| " | ||
| `) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The stabilize helper rounds the measured time to the nearest 10ms step, so [510.00ms] is only stable if the setTimeout(r, 500) call fires between 500.001ms and 510ms. Under CI load, timers can fire well after 510ms (e.g., 515ms → rounds to 520ms), causing the snapshot to fail. Consider widening the tolerance — e.g., replacing ceil with a coarser step (50ms or 100ms) — or using a spy/mock for the timer instead of a real 500ms sleep.
| span<T>(label: string, fn: () => T): T { | ||
| this.start(label) | ||
| let isPromise = false | ||
| try { | ||
| let result = fn() | ||
|
|
||
| isPromise = result && typeof (result as any).then === 'function' | ||
|
|
||
| // @ts-expect-error — TS can't infer that result is a Promise here | ||
| return isPromise ? result.finally(() => this.end(label)) : result | ||
| } finally { | ||
| if (!isPromise) this.end(label) | ||
| } | ||
| } |
There was a problem hiding this comment.
start(label) pushes onto the shared #timerStack. If an async span is not awaited before another span is started, by the time the async .finally() fires and calls this.end(label), a different label sits at the top of the stack and end() throws "Mismatched timer label". For example: const p = I.span('Foo', async () => { await delay() }); I.span('Bar', () => {}); await p — end('Foo') will throw because Bar was interleaved. A brief doc-comment warning about this constraint would save confusion for future contributors.
WalkthroughThis PR extends the 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/`@tailwindcss-node/src/instrumentation.ts:
- Around line 72-85: span() marks any thenable as a promise but calls
result.finally(...) which throws for thenables without finally; change the logic
so after detecting a thenable (isPromise when result && typeof result.then ===
'function') you return Promise.resolve(result).finally(() => this.end(label))
instead of calling result.finally directly, and keep the existing finally block
to call this.end(label) only when not isPromise; update the return path in
span<T> (function span) to wrap thenables with Promise.resolve(...) so timers
are always balanced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1fd9164d-4368-4411-b231-f60e7a5bce77
📒 Files selected for processing (2)
packages/@tailwindcss-node/src/instrumentation.test.tspackages/@tailwindcss-node/src/instrumentation.ts
| span<T>(label: string, fn: () => T): T { | ||
| this.start(label) | ||
| let isPromise = false | ||
| try { | ||
| let result = fn() | ||
|
|
||
| isPromise = result && typeof (result as any).then === 'function' | ||
|
|
||
| // @ts-expect-error — TS can't infer that result is a Promise here | ||
| return isPromise ? result.finally(() => this.end(label)) : result | ||
| } finally { | ||
| if (!isPromise) this.end(label) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, can a Promise-like thenable implement thenwithout implementingfinally?
💡 Result:
Yes. A thenable can implement only then and does not need to implement finally(). Evidence: - The “thenable protocol” / interop is driven by .then(). MDN notes that the thenable protocol “expects all promise-like objects to expose a then() method”.[1] - Promises/A+ defines a thenable as “an object or function that defines a then method”, and the promise resolution procedure assimilates thenables by reading x.then and calling it.[2] - Promise.prototype.finally() works because it calls .then on the promise it was invoked on (“finally() internally calls the then method on the object upon which it was called”).[3] So, if your object only has then, it can still be used with await and .then(...) chains, because those operations rely on then being present.[1][2] But you can’t call .finally(...) directly on that object unless it also has a finally method; instead you can wrap it, e.g. Promise.resolve(thenable).finally(...), because Promise.resolve(...) returns a real Promise that has finally().[3]
Citations:
- 1: https://github.com/mdn/content/blob/main/files/en-us/web/javascript/reference/global_objects/promise/then/index.md
- 2: https://github.com/promises-aplus/promises-spec/blob/master/README.md
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then
Fix span() to handle thenables without .finally
span() treats any value with then as promise-like, but calls result.finally(...) directly; a thenable may omit finally, causing the throw to bypass this.end(label) (since isPromise is true) and leave timers unbalanced.
Suggested fix
span<T>(label: string, fn: () => T): T {
this.start(label)
let isPromise = false
try {
let result = fn()
isPromise = result && typeof (result as any).then === 'function'
- // `@ts-expect-error` — TS can't infer that result is a Promise here
- return isPromise ? result.finally(() => this.end(label)) : result
+ // Normalize thenables so `.finally` is always available.
+ // `@ts-expect-error` — TS can't infer the conditional return shape here
+ return isPromise ? Promise.resolve(result).finally(() => this.end(label)) : result
} finally {
if (!isPromise) this.end(label)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| span<T>(label: string, fn: () => T): T { | |
| this.start(label) | |
| let isPromise = false | |
| try { | |
| let result = fn() | |
| isPromise = result && typeof (result as any).then === 'function' | |
| // @ts-expect-error — TS can't infer that result is a Promise here | |
| return isPromise ? result.finally(() => this.end(label)) : result | |
| } finally { | |
| if (!isPromise) this.end(label) | |
| } | |
| } | |
| span<T>(label: string, fn: () => T): T { | |
| this.start(label) | |
| let isPromise = false | |
| try { | |
| let result = fn() | |
| isPromise = result && typeof (result as any).then === 'function' | |
| // Normalize thenables so `.finally` is always available. | |
| // `@ts-expect-error` — TS can't infer the conditional return shape here | |
| return isPromise ? Promise.resolve(result).finally(() => this.end(label)) : result | |
| } finally { | |
| if (!isPromise) this.end(label) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/`@tailwindcss-node/src/instrumentation.ts around lines 72 - 85,
span() marks any thenable as a promise but calls result.finally(...) which
throws for thenables without finally; change the logic so after detecting a
thenable (isPromise when result && typeof result.then === 'function') you return
Promise.resolve(result).finally(() => this.end(label)) instead of calling
result.finally directly, and keep the existing finally block to call
this.end(label) only when not isPromise; update the return path in span<T>
(function span) to wrap thenables with Promise.resolve(...) so timers are always
balanced.
1 participant
This PR improves some of the internal instrumentation tooling we have.
While working on another branch, I updated the instrumentation tooling to have a few different ways of measuring what's going on.
Until now, we had an
I.start(label)and correspondingI.end(label). While this works, it also means that you have to make sure that you callI.end(label)before everyreturnto track things properly.With this PR, I added a
I.span(label, () => /* some callback*/{})API that essentially does that in one go. It also handles promises and resturns the value that was returned from the callback. This can be useful in situations where you have a one-liner:If your callback is longer, then you end up in a situation where you have to indent your code, and if you want to stop measuring you have to drop code in 2 places and re-indent:
For this situation, I also added a
using _ = I.track(label)API instead. This can also be used in any block and automatically inserts theI.end(label)on every exit point. This relies on the newusingkeyword, but we already relied on that for the instrumentation module.Last but not least, the constructor accepts a
shouldReportwhich defaults to theenv.DEBUG. The reason for this change is so that it's easier to report / not report during development instead of swapping out an environment variable. Again, this is internal so there is no public API change happening here.Test plan
All tests should still pass.