feat: display 'Player #' in logs to distinguish log messages from different Conductors - #302
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![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 (2)
ts/src/conductor.ts(18 hunks)ts/src/scenario.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ts/src/scenario.ts (1)
ts/src/conductor.ts (1)
NetworkConfig(66-135)
ts/src/conductor.ts (1)
ts/src/logger.ts (1)
makeLogger(9-27)
⏰ 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 (13)
ts/src/scenario.ts (4)
46-46: LGTM!Adding an optional
labelfield toAppWithOptionsis a clean way to support labeled conductors.
229-231: LGTM!Correctly passes the label from
appWithOptionstoaddConductor.
285-287: LGTM!The map callback correctly uses the index parameter
ito generate player labels.
105-127: AlladdConductorcallsites match the new(label?, networkConfig?)signature.
No callers pass aNetworkConfigas the first argument.ts/src/conductor.ts (9)
57-60: LGTM!Adding an optional
labelfield toConductorOptionsprovides clear documentation and type safety for the labeling feature.
169-172: LGTM!Correctly includes
labelin theCreateConductorOptionstype alongside other conductor configuration options.
224-232: LGTM!The constructor signature change is safe since it's private, and the instance logger is correctly initialized with the provided label.
250-274: LGTM!Correctly uses
defaultLoggerin the static factory method since instance context isn't available yet. The label is passed to the constructor at line 255.
310-312: LGTM!Correctly uses
this._loggerfor instance logging, ensuring the conductor's label appears in network config update logs.
325-395: LGTM!All logging calls consistently use
this._loggerthroughout the startup and shutdown methods, ensuring proper label propagation.
403-463: LGTM!Instance logger usage is consistent across all websocket connection methods.
508-513: LGTM!Correctly uses
this._loggerfor app installation logging.
537-537: LGTM!Correctly uses
defaultLoggerin the standalonecleanAllConductorsfunction since there's no instance context.
…ferent conductors
9344617 to
adcdcaa
Compare
…ferent conductors
df7b611 to
fc8acc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ts/src/scenario.ts (1)
145-147: Index usage fix acknowledged.Using the index parameter ensures unique labels per player. Good catch.
🧹 Nitpick comments (5)
ts/src/conductor.ts (3)
24-24: Use a type-only import for Logger.Avoids an unnecessary runtime import.
-import { Logger } from "winston"; +import type { Logger } from "winston";
221-231: Make the instance logger readonly.Prevents accidental reassignment.
- private _logger: Logger; + private readonly _logger: Logger;
250-271: Route creation-time logs through the instance logger (with label).This preserves the conductor label in early logs and meets the PR’s goal more completely. Create the instance before logging and replace defaultLogger with the instance logger in the static create flow.
- args.push(signalingServerUrl.href); - defaultLogger.debug("spawning hc sandbox with args:", args); - const createConductorProcess = spawn("hc", args); + args.push(signalingServerUrl.href); + const conductor = new Conductor(options?.label, options?.timeout); + conductor._logger.debug("spawning hc sandbox with args:", args); + const createConductorProcess = spawn("hc", args); @@ - const conductor = new Conductor(options?.label, options?.timeout); return new Promise<Conductor>((resolve, reject) => { createConductorProcess.stdout.on("data", (data: Buffer) => { - defaultLogger.debug(`creating conductor config\n${data.toString()}`); + conductor._logger.debug(`creating conductor config\n${data.toString()}`); @@ - createConductorProcess.stderr.on("data", (err) => { - defaultLogger.error(`error when creating conductor config: ${err}\n`); + createConductorProcess.stderr.on("data", (err) => { + conductor._logger.error(`error when creating conductor config: ${err}\n`); reject(err); }); });ts/src/scenario.ts (1)
266-271: Consider 1‑based player labels.For human readability, “Player 1..N” may be preferable to “Player 0..N-1”. Optional.
- new Array(amount).fill(0).map((_, i) => - this.addPlayerWithApp({ - label: `Player ${i}`, // default label + new Array(amount).fill(0).map((_, i) => + this.addPlayerWithApp({ + label: `Player ${i + 1}`, // default label ...appWithOptions, }), ),- appsWithOptions.map((appWithOptions, i) => - this.addPlayerWithApp({ - label: `Player ${i}`, // default label + appsWithOptions.map((appWithOptions, i) => + this.addPlayerWithApp({ + label: `Player ${i + 1}`, // default label ...appWithOptions, }), ),Also applies to: 285-290
docs/tryorama.appwithoptions.md (1)
53-72: Ignore MD lint warning for generated docs (or exclude docs from lint).MD036 (“Emphasis used instead of a heading”) is triggered by API Documenter’s “(Optional)” emphasis. Prefer excluding generated docs from markdownlint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 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)
✅ Files skipped from review due to trivial changes (2)
- docs/tryorama.conductoroptions.md
- docs/tryorama.appwithoptions.label.md
🧰 Additional context used
🧬 Code graph analysis (2)
ts/src/scenario.ts (1)
ts/src/conductor.ts (1)
NetworkConfig(66-135)
ts/src/conductor.ts (1)
ts/src/logger.ts (1)
makeLogger(9-27)
🪛 markdownlint-cli2 (0.18.1)
docs/tryorama.appwithoptions.md
69-69: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (1)
docs/tryorama.conductoroptions.label.md (1)
1-13: Docs entry looks good; ensure docs build step is completed.Content is correct. Please run the documented build step and commit updated docs before merge.
mattyg
changed the title
mattyg
changed the title
…y created players' label is incremented starting from number of existing players
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
10-10: Tighten the relative clauseConsider inserting a comma before “which” so the sentence reads more cleanly: “It allows overriding the default label, which will prefix log messages …”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)ts/src/scenario.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ts/src/scenario.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~12-~12: There might be a mistake here.
Context: ...of that Conductor instance. ### Removed ### Changed - Log messages from Conductors t...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...uctor instance. ### Removed ### Changed - Log messages from Conductors that were c...
(QB_NEW_EN)
⏰ 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)
|
✔️ adcdcaa...1b352a2 - Conventional commits check succeeded. |
|
I merged |
…eation and app install sequential (#303) * fix: avoid test failures due to apparant connection race condition -- start each player's conductor sequentially after a 1 second delay and adjust default k2Gossip configuration * chore: lint * docs: regenerate * chore: comment uniformity * test: update default config test * test: remove test that can no longer be achieved, since integration_limbo empties too quickly, instead cover assertions in success case test * chore: lint * test: remove test that can no longer be achieved, since integration_limbo empties too quickly, instead cover assertions in success case test * fix: reading ADMIN_PORT from conductor stdout was flaky * chore: changelog grammer * feat: display 'Player #' in logs to distinguish log messages from different Conductors (#302) * feat: display 'Player #' in logs to distinguish log messages from different conductors * feat: display 'Player #' in logs to distinguish log messages from different conductors * docs: rebuild docs * chore: grammer * fix: if addPlayersX is called multiple times, ensure that subsequently created players' label is incremented starting from number of existing players * chore: lint * chore: Revert " feat: display 'Player #' in logs to distinguish log messages from di…" (#306) This reverts commit 20ee01b. * revert: remove pause between conductor starts from addPlayers -- only need to stagger app installation as that initiates networking * chore: add TODO to comments explaining workaround * chore: clarifying comment * fix: clone rather than mutate map param * chore: lint * chore: removed TODO comment, which actually was not related to workaround * chore: unrelated typo * refactor: actually install apps sequentially, rather than via arbitrary pause * chore: lint * docs: clarify that addPlayerWithApp should not be called in parallel * chore: changelog * chore: changelog accuracy
3 participants
Summary
Adds a "label" string to the prefix string in conductor log messages. For conductors created by the
addPlayers,addPlayersWithApps, oraddPlayersWithSameAppfunctions, the label defaults to "Player X" (where X is the index of when the player was created, starting from 0).I'm not sure of a good way to test this. If others think its important to have tests on it I can dig in further.
(merging into fix/dht-sync-timing-out for a clean diff)
TODO:
npm run build && git add docs)Summary by CodeRabbit
New Features
Refactor