Build: Fix async telemetry event sending #33115
Conversation
valentinpalkovic
force-pushed
the
valentin/fix-telemetry-event-sending-for-build
branch
from
eab9e7e to
83d5bd2
Compare
📝 WalkthroughWalkthroughTelemetry in build-static.ts was changed from a deferred side-effect queued in an effects array to an explicit, awaited operation; a typed payload object is constructed (conditionally including a summarized Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Build as Build Process
participant Telemetry as Telemetry Service
rect rgb(200,220,240)
Note over Build,Telemetry: Before — Deferred effect (non-blocking)
Build->>Build: Create telemetry task (deferred)
Build->>Build: Push task into effects array
Note over Build: Continue without awaiting
Build->>Telemetry: Effects executed asynchronously later
end
rect rgb(220,240,200)
Note over Build,Telemetry: After — Awaited, sequential
Build->>Build: Create payload object (include precedingUpgrade)
alt storyIndex present
Build->>Build: Add summarized storyIndex to payload
end
Build->>Telemetry: Await init() then send(payload)
Telemetry-->>Build: Success or throws
opt error
Note over Build: Caught and logged at debug level (does not fail build)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/core-server/build-static.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
code/core/src/core-server/build-static.ts (2)
code/core/src/core-server/presets/common-preset.ts (1)
core(190-195)code/core/src/telemetry/index.ts (1)
telemetry(29-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
| storyIndex: summarizeIndex(storyIndex), | ||
| }); | ||
| } | ||
| await initializedStoryIndexGenerator.then(async (generator) => { |
There was a problem hiding this comment.
@shilman are you ok with this change? I assume this wasn't awaited for a reason (likely performance reasons)
There was a problem hiding this comment.
@shilman are you ok with this change? I assume this wasn't awaited for a reason (likely performance reasons)
So let’s quickly go through it:
Even if you don’t await the telemetry event, your process will hang until it is resolved and the process can exit.
So currently you will see this in the terminal:
|
|> DONE
// SEND TELEMETRY (process doesn't finish until done)
| Attention: Telemetry notification....
// PROCESS EXITS
After the change:
|
// SEND TELEMETRY
| Attention: Telemetry notification....
|> DONE
// PROCESS EXITS IMMEDIATELY
Now, if Done is displayed, the process finishes immediately afterwards. The situation before was more frustrating, because the build finished, done was displayed, but because of telemetry, the process didn’t finish immediately afterwards
There was a problem hiding this comment.
@valentinpalkovic how does this change impact time to first render?
There was a problem hiding this comment.
We are in build, not dev
|
View your CI Pipeline Execution ↗ for commit 17d46c9
☁️ Nx Cloud last updated this comment at |
valentinpalkovic
force-pushed
the
valentin/fix-telemetry-event-sending-for-build
branch
from
83d5bd2 to
67f01ea
Compare
…d await before final log to avoid post-build UI glitches
valentinpalkovic
force-pushed
the
valentin/fix-telemetry-event-sending-for-build
branch
from
67f01ea to
17d46c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/core-server/build-static.ts (1)
243-243: Consider using thePayloadtype instead ofany.The
payload: anydeclaration loses type safety. Sincetelemetryis already imported fromstorybook/internal/telemetry, consider importing and using thePayloadtype for better type checking.Apply this diff to improve type safety:
+import { getPrecedingUpgrade, telemetry, type Payload } from 'storybook/internal/telemetry';Then update the payload declaration:
- const payload: any = { + const payload: Payload = { precedingUpgrade: await getPrecedingUpgrade(), };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/core-server/build-static.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
code/core/src/core-server/build-static.ts (2)
code/core/src/core-server/presets/common-preset.ts (1)
core(190-195)code/core/src/telemetry/index.ts (1)
telemetry(29-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/core-server/build-static.ts (1)
240-256: Good addition of error handling for telemetry.The try-catch wrapper ensures that telemetry failures won't break the build, and logging at debug level is appropriate for best-effort telemetry. The direct await pattern is cleaner and more readable than chaining with
.then().
valentinpalkovic
deleted the
valentin/fix-telemetry-event-sending-for-build
branch
Closes #
What I did
I fixed an issue where sending the telemetry event wasn't properly awaited, and its log was triggered after the whole build process actually had finished.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Refactor
Bug Fix
✏️ Tip: You can customize this high-level summary in your review settings.