Feat/player id in logs - #307
Conversation
…ferent conductors
…ferent conductors
…y created players' label is incremented starting from number of existing players
… start each player's conductor sequentially after a 1 second delay and adjust default k2Gossip configuration
…imbo empties too quickly, instead cover assertions in success case test
…imbo empties too quickly, instead cover assertions in success case test
WalkthroughAdded an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e5c82e4 to
70820bc
Compare
![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
🧹 Nitpick comments (1)
docs/tryorama.appwithoptions.label.md (1)
1-11: Documentation lacks a description of the label field.Unlike the corresponding ConductorOptions.label documentation page (which includes "Label to identify this conductor in logs"), this page for AppWithOptions.label only provides the type signature without explanation. Consider adding a brief description to clarify the purpose of this field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(2 hunks)docs/tryorama.appwithoptions.label.md(1 hunks)docs/tryorama.appwithoptions.md(1 hunks)docs/tryorama.conductoroptions.label.md(1 hunks)docs/tryorama.conductoroptions.md(1 hunks)docs/tryorama.createconductoroptions.md(1 hunks)docs/tryorama.scenario.addconductor.md(2 hunks)docs/tryorama.scenario.md(1 hunks)ts/src/conductor.ts(18 hunks)ts/src/scenario.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ts/src/conductor.ts (1)
ts/src/logger.ts (1)
makeLogger(9-27)
ts/src/scenario.ts (1)
ts/src/conductor.ts (1)
NetworkConfig(66-135)
🪛 markdownlint-cli2 (0.18.1)
docs/tryorama.appwithoptions.md
69-69: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/tryorama.scenario.addconductor.md
61-61: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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: test (ubuntu-latest)
- GitHub Check: test (macos-latest)
🔇 Additional comments (29)
docs/tryorama.appwithoptions.md (1)
54-72: Auto-generated documentation is consistent with existing patterns.The label property addition follows the same structure and format as other optional properties in AppWithOptions. The table row is correctly formatted and consistent with the generated documentation style.
docs/tryorama.conductoroptions.label.md (1)
1-12: New property documentation is well-formed and clear.The new file follows standard API Documenter conventions with a clear description and correct TypeScript signature for the optional label property.
docs/tryorama.createconductoroptions.md (1)
12-12: Type definition correctly reflects label addition to conductor options.The CreateConductorOptions type now includes "label" in its Pick from ConductorOptions, enabling label specification during conductor creation.
docs/tryorama.scenario.addconductor.md (2)
12-12: Method signature correctly updated to include optional label parameter.The addConductor signature now includes the optional label parameter, reflecting the new capability to customize conductor labels.
49-64: Parameter documentation added with consistent formatting.The label parameter is documented in the parameters table following the same format as networkConfig, clearly indicating it is optional.
docs/tryorama.scenario.md (1)
230-230: Method reference correctly updated to reflect label parameter.The Scenario.addConductor method reference now shows both parameters, maintaining consistency with the detailed method documentation.
docs/tryorama.conductoroptions.md (1)
56-74: Property documentation consistently added to ConductorOptions interface.The label property is properly integrated into the ConductorOptions documentation with a clear description and appropriate reference link, and is positioned logically in the properties table.
CHANGELOG.md (2)
8-8: Changelog entry accurately documents the new label feature.The addition properly summarizes the new optional label field across the affected types and function, with a clear explanation of its purpose in log message prefixing.
13-14: Changelog entries comprehensively document behavioral changes.The changelog properly documents:
- The new label field addition (line 8)
- Sequential conductor creation with delays (lines 24, 28)
- Player index in log prefixes via "Tryorama - Player #" (line 24)
- Fixes for flaky startup recognition (line 14, 29)
All entries are well-organized with appropriate PR references.
Also applies to: 24-24, 28-29
ts/src/scenario.ts (5)
47-47: LGTM!The optional
labelfield allows callers to provide custom conductor labels, which integrates cleanly with the conductor creation flow.
106-114: LGTM!The
addConductormethod correctly accepts and forwards the optional label to the conductor creation options.
149-150: Label generation is safe for parallel creation.The
generatePlayerLabelmethod usesthis.conductors.length + index, which is evaluated when called. Since the loop indexiincrements andthis.conductors.lengthis captured at the time of the map, labels will be consecutive (e.g., "Player 0", "Player 1", "Player 2") even when conductors are created in parallel viaPromise.all.
266-266: LGTM!The label from
appWithOptionsis correctly passed toaddConductor.
388-390: LGTM!The
generatePlayerLabelmethod produces human-readable labels in the format "Player N" where N is the player's index in the scenario.ts/src/conductor.ts (15)
24-26: LGTM!The
defaultLoggeris appropriately used for static and pre-instance logging contexts where a per-instance logger is not yet available.
57-60: LGTM!The
labelfield is well-documented and provides a clear way to identify conductors in logs.
169-172: LGTM!Including
labelin theCreateConductorOptionstype ensures it can be passed through the factory function.
186-190: LGTM!The
labelis correctly picked from options and passed to the conductor factory.
224-232: Constructor signature change is safe.The constructor parameter order changed from
(timeout?: number)to(label?: string, timeout?: number). Since the constructor is private and only called by the factory method (line 255), this is not a breaking change for external consumers.
250-273: LGTM!The
defaultLoggeris appropriately used during conductor creation since the instance logger isn't available until after the constructor runs.
310-312: LGTM!Using the instance logger (
this._logger) for configuration updates correctly associates these logs with the specific conductor instance.
325-328: LGTM!The error message now uses the instance logger, improving log clarity when multiple conductors are running.
344-359: LGTM!Startup logs now use the instance logger, making it easy to track which conductor is starting up and identify issues per instance.
372-403: LGTM!Shutdown logs use the instance logger, providing clear visibility into which conductor is shutting down.
411-411: LGTM!Connection logs use the instance logger for better traceability.
425-426: LGTM!App interface attachment logs use the instance logger.
438-438: LGTM!App WebSocket connection logs use the instance logger.
516-520: LGTM!App installation logs use the instance logger, making it clear which conductor is installing which app.
545-545: LGTM!The static cleanup function correctly uses
defaultLoggersince it operates globally across all conductors.
|
✔️ adcdcaa...1161111 - Conventional commits check succeeded. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ts/src/scenario.ts (1)
135-157: Documentation-code mismatch: Implementation is parallel, not sequential.The documentation states "Each conductor is created sequentially, once the previous has completed startup" (lines 135-136), but the implementation uses
Promise.all, which creates conductors in parallel.Additionally, multiple other methods in this file (
installAppsForPlayers,installSameAppForPlayers,addPlayersWithSameApp,addPlayersWithApps) explicitly use sequentialforloops with TODO comments referencing issue #297 to avoid connection failures. This method should follow the same pattern.Apply this diff to make conductor creation sequential and consistent with other methods:
async addPlayers( amount: number, networkConfig?: NetworkConfig, ): Promise<Player[]> { await this.ensureLocalServices(); - return Promise.all( - new Array(amount).fill(0).map(async (_, i) => { - const conductor = await this.addConductor( - networkConfig, - this.generatePlayerLabel(i), - ); - const agentPubKey = await conductor.adminWs().generateAgentPubKey(); - return { conductor, agentPubKey }; - }), - ); + // Sequentially create conductors. + // TODO This is a workaround to avoid connection failures. + // See https://github.com/holochain/tryorama/issues/297 + const players = []; + for (let i = 0; i < amount; i++) { + const conductor = await this.addConductor( + networkConfig, + this.generatePlayerLabel(i), + ); + const agentPubKey = await conductor.adminWs().generateAgentPubKey(); + players.push({ conductor, agentPubKey }); + } + return players; }
🧹 Nitpick comments (1)
ts/src/scenario.ts (1)
390-392: Consider 1-indexed labels for better UX.The current implementation generates 0-indexed labels ("Player 0", "Player 1", etc.). For human-readable log messages, 1-indexed labels ("Player 1", "Player 2", etc.) are typically more intuitive and align with common UX conventions.
Apply this diff to use 1-indexed labels:
private generatePlayerLabel(index: number): string { - return `Player ${this.conductors.length + index}`; + return `Player ${this.conductors.length + index + 1}`; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)ts/src/scenario.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (1)
ts/src/scenario.ts (1)
ts/src/conductor.ts (1)
NetworkConfig(66-135)
🔇 Additional comments (3)
ts/src/scenario.ts (3)
46-46: LGTM! Label field properly added.The optional
labelfield is correctly typed and follows TypeScript conventions for optional properties.
105-127: LGTM! Label parameter properly propagated.The
labelparameter is correctly threaded through the conductor creation options and handles both configuration scenarios appropriately.
264-275: LGTM! Label correctly propagated from app options.The
appWithOptions.labelis properly passed toaddConductor, maintaining consistency with the label propagation design.
2 participants
Summary
Display Player index in conductor log messages
(had to recreate this PR branch after accidentally merging it into another PR branch)
TODO:
npm run build && git add docs)Summary by CodeRabbit
New Features
labelparameter to customize conductor identifiers in logs.Bug Fixes
Documentation
labelproperty across interfaces and methods.