fix(logger): skip profiling when the level is off, bound the profiler state by davidfirst · Pull Request #10677 · teambit/bit

davidfirst · 2026-08-31T20:16:53Z

The logger profiler has two problems.

The log level controls only the printing. profile() always records the time and builds the message. Therefore a disabled level does not prevent the work. The method now returns before it records the time, if the level is off. The --log=profile flag and the console argument continue to work. The default level is debug. Therefore all profileTrace() calls are now free.

profile() is a paired API. The daemon changes the level between CLI requests, therefore a measurement can stay open. A later call with the same id would then close it and report an incorrect time. The disabled call now discards the measurement of its own id. It does not touch the other ids, because requests can run in parallel.

The profiler state increases without a limit. The profiler keeps one entry for each id, and it removes no entry. An id that changes for each call (for example getMany-${callId}) makes the state increase for the life of the process. The bit cli daemon does not stop, which makes this worse.

The profiler now keeps the running measurements apart from the completed ones. Each group holds a maximum of 10000 entries. Each group removes its oldest entry to make space. Therefore a new measurement never removes a measurement that still runs. The limit is sufficient for real use: 300 components with 5 profile points on each need 1500 entries.

profiler.spec.ts adds 9 unit tests.

…s map

qodo-free-for-open-source-projects · 2026-08-31T20:17:34Z

PR Summary by Qodo

Skip disabled profiling and bound retained profiler state

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Skip profiling state and formatting when target log output is disabled.
• Cap retained profiler IDs at 500, prioritizing completed entries for eviction.
• Test timing semantics, ID isolation, accumulation, and bounded retention.

Diagram

graph TD
  Call["Profile Call"] --> Gate{"Output enabled?"} -->|Yes| Profiler["Profiler"] --> State["Bounded State"]
  Gate -->|No| Skip["Skip Work"]
  Profiler -->|Measurement complete| Output["Console or Log"]

High-Level Assessment

The following are alternative approaches to this PR:

1. Time-based expiration

➕ Naturally removes abandoned and inactive measurements
➕ Can retain frequently reused IDs beyond a fixed capacity
➖ Requires expiration timestamps or periodic cleanup
➖ Introduces time-dependent behavior and harder tests

2. Strict LRU retention

➕ Keeps the most recently used profiler IDs
➕ Provides a familiar bounded-cache policy
➖ May evict an active long-running measurement
➖ Requires refreshing Map order or a cache abstraction

Recommendation: Keep the completed-first fixed-cap approach: it is deterministic, dependency-free, and protects active measurements under normal pressure. Time-based cleanup would better guarantee removal of abandoned open entries, but adds lifecycle complexity that is disproportionate to this focused logger fix.

Files changed (3) +112 / -10

Bug fix (2) +52 / -10

logger.tsBypass profiling when its output level is disabled +6/-1

Bypass profiling when its output level is disabled

• Checks console overrides and logger-level enablement before recording timestamps or formatting profiling messages. Explicit console profiling and the global profiler-console option continue to bypass the level gate.

components/legacy/logger/logger.ts

profiler.tsBound profiler state with completed-first eviction +46/-9

Bound profiler state with completed-first eviction

• Replaces the unbounded object with a Map capped at 500 IDs and exposes its retained size for verification. New IDs evict the oldest completed measurement first, falling back to the oldest active entry when necessary.

components/legacy/logger/profiler.ts

Tests (1) +60 / -0

profiler.spec.tsCover profiler measurement and retention behavior +60/-0

Cover profiler measurement and retention behavior
• Adds five unit tests for opening and closing measurements, cumulative totals, ID isolation, capacity enforcement, and completed-first eviction.
components/legacy/logger/profiler.spec.ts

qodo-free-for-open-source-projects · 2026-08-31T20:18:39Z

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. ~~Active profiles evicted prematurely~~ ✓ Resolved 🐞 Bug ◔ Observability

Description

evictIfNeeded() stops after 50 open entries and then deletes the oldest active profiler even when
completed profilers exist later in the map. The evicted profile's closing call is treated as a new
opening call, so its timing result is silently lost.

Code

components/legacy/logger/profiler.ts[R71-74]

+      scanned += 1;
+      if (scanned >= EVICTION_SCAN_LIMIT) break;
+    }
+    if (oldestId !== undefined) this.profilers.delete(oldestId);

Evidence
New IDs are appended to an insertion-ordered Map, while completed entries remain at their original
positions. The eviction loop examines only 50 entries and then unconditionally removes the oldest,
despite the public API requiring paired calls and the added test explicitly requiring long-running
open measurements to survive completed ones.
components/legacy/logger/profiler.ts[19-19]
components/legacy/logger/profiler.ts[31-44]
components/legacy/logger/profiler.ts[61-74]
components/legacy/logger/profiler.spec.ts[49-58]
components/legacy/logger/logger.ts[194-218]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The bounded profiler can evict an active measurement after scanning only 50 entries even though a completed entry is available later, causing the active measurement's eventual closing call to produce no result.
## Issue Context
`Map` iteration follows insertion order, while closing a profiler does not move it. Therefore completed candidates can legitimately occur beyond the first 50 entries.
## Fix Focus Areas
- components/legacy/logger/profiler.ts[61-74]
- components/legacy/logger/profiler.spec.ts[49-58]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Logger switches erase concurrent profiles 🐞 Bug ◔ Observability

Description

Every logger switch now clears the process-global profiler, so an overlapping API request can erase
measurements opened by a request that is still running. Those measurements' closing calls then
become new opening calls and emit no timing result.

Code

components/legacy/logger/logger.ts[335]
+    this.profiler.reset();

Evidence
BitLogger owns one profiler, and the API route mutates that shared logger around an awaited
command. Since all switch methods now funnel through switchTo() and clear the profiler,
interleaved requests can clear each other's profiling state.
components/legacy/logger/logger.ts[76-98]
components/legacy/logger/logger.ts[314-335]
scopes/harmony/api-server/cli-raw.route.ts[100-109]
scopes/harmony/api-server/cli-raw.route.ts[125-170]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Unconditionally resetting the singleton profiler during logger switches loses open measurements belonging to concurrently running daemon/API requests.
## Issue Context
The CLI raw route switches the shared legacy logger before asynchronous command execution and restores it in `finally`, allowing another overlapping request's switch or restore to reset profiling state mid-measurement.
## Fix Focus Areas
- components/legacy/logger/logger.ts[326-335]
- scopes/harmony/api-server/cli-raw.route.ts[100-109]
- scopes/harmony/api-server/cli-raw.route.ts[125-170]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. ~~Level changes corrupt profiles~~ ✓ Resolved 🐞 Bug ◔ Observability

Description

The new disabled-level return skips the closing state transition for an already-open profile, so if
the logger level changes between paired calls, the next enabled call closes the stale measurement
and reports time across unrelated work. This is reachable because the daemon swaps the global legacy
logger and its level around CLI requests.

Code

components/legacy/logger/logger.ts[214]
+    if (!shouldWriteToConsole && !this.logger.isLevelEnabled(level)) return;

Evidence
The documented API opens and closes a measurement with two calls, and Profiler removes current
only when another call reaches its closing branch. The changed return prevents that branch from
running, while the daemon route changes the shared logger before a command and restores it
afterward, making level changes during the profiler's lifetime a supported runtime behavior.
components/legacy/logger/logger.ts[194-218]
components/legacy/logger/profiler.ts[18-35]
scopes/harmony/api-server/cli-raw.route.ts[100-110]
scopes/harmony/api-server/cli-raw.route.ts[145-171]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A profile opened while its level is enabled remains open if its matching call occurs after that level is disabled. A later enabled call then closes stale state and emits an invalid duration.
## Issue Context
`profile()` is a paired-call API, while the daemon can switch the shared legacy logger and level around requests. Disabled calls should not format messages or record timestamps, but they must not preserve an earlier open interval.
## Fix Focus Areas
- components/legacy/logger/logger.ts[209-218]
- components/legacy/logger/profiler.ts[18-35]
- scopes/harmony/api-server/cli-raw.route.ts[100-110]
- scopes/harmony/api-server/cli-raw.route.ts[169-171]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 98c3232 ⚖️ Balanced

Results up to commit 09b41e0

🐞 Bugs (3) 📘 Rule violations (0) 📜 Skill insights (0)

1. Active profiles evicted prematurely 🐞 Bug ◔ Observability ⭐ New

Description

evictIfNeeded() stops after 50 open entries and then deletes the oldest active profiler even when
completed profilers exist later in the map. The evicted profile's closing call is treated as a new
opening call, so its timing result is silently lost.

Code

components/legacy/logger/profiler.ts[R71-74]

+      scanned += 1;
+      if (scanned >= EVICTION_SCAN_LIMIT) break;
+    }
+    if (oldestId !== undefined) this.profilers.delete(oldestId);

Evidence
New IDs are appended to an insertion-ordered Map, while completed entries remain at their original
positions. The eviction loop examines only 50 entries and then unconditionally removes the oldest,
despite the public API requiring paired calls and the added test explicitly requiring long-running
open measurements to survive completed ones.
components/legacy/logger/profiler.ts[19-19]
components/legacy/logger/profiler.ts[31-44]
components/legacy/logger/profiler.ts[61-74]
components/legacy/logger/profiler.spec.ts[49-58]
components/legacy/logger/logger.ts[194-218]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The bounded profiler can evict an active measurement after scanning only 50 entries even though a completed entry is available later, causing the active measurement's eventual closing call to produce no result.

## Issue Context
`Map` iteration follows insertion order, while closing a profiler does not move it. Therefore completed candidates can legitimately occur beyond the first 50 entries.

## Fix Focus Areas
- components/legacy/logger/profiler.ts[61-74]
- components/legacy/logger/profiler.spec.ts[49-58]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Logger switches erase concurrent profiles 🐞 Bug ◔ Observability ⭐ New

Description

Every logger switch now clears the process-global profiler, so an overlapping API request can erase
measurements opened by a request that is still running. Those measurements' closing calls then
become new opening calls and emit no timing result.

Code

components/legacy/logger/logger.ts[335]
+    this.profiler.reset();

Evidence
BitLogger owns one profiler, and the API route mutates that shared logger around an awaited
command. Since all switch methods now funnel through switchTo() and clear the profiler,
interleaved requests can clear each other's profiling state.
components/legacy/logger/logger.ts[76-98]
components/legacy/logger/logger.ts[314-335]
scopes/harmony/api-server/cli-raw.route.ts[100-109]
scopes/harmony/api-server/cli-raw.route.ts[125-170]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Unconditionally resetting the singleton profiler during logger switches loses open measurements belonging to concurrently running daemon/API requests.

## Issue Context
The CLI raw route switches the shared legacy logger before asynchronous command execution and restores it in `finally`, allowing another overlapping request's switch or restore to reset profiling state mid-measurement.

## Fix Focus Areas
- components/legacy/logger/logger.ts[326-335]
- scopes/harmony/api-server/cli-raw.route.ts[100-109]
- scopes/harmony/api-server/cli-raw.route.ts[125-170]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Level changes corrupt profiles 🐞 Bug ◔ Observability

Description

The new disabled-level return skips the closing state transition for an already-open profile, so if
the logger level changes between paired calls, the next enabled call closes the stale measurement
and reports time across unrelated work. This is reachable because the daemon swaps the global legacy
logger and its level around CLI requests.

Code

components/legacy/logger/logger.ts[214]
+    if (!shouldWriteToConsole && !this.logger.isLevelEnabled(level)) return;

Evidence
The documented API opens and closes a measurement with two calls, and Profiler removes current
only when another call reaches its closing branch. The changed return prevents that branch from
running, while the daemon route changes the shared logger before a command and restores it
afterward, making level changes during the profiler's lifetime a supported runtime behavior.
components/legacy/logger/logger.ts[194-218]
components/legacy/logger/profiler.ts[18-35]
scopes/harmony/api-server/cli-raw.route.ts[100-110]
scopes/harmony/api-server/cli-raw.route.ts[145-171]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A profile opened while its level is enabled remains open if its matching call occurs after that level is disabled. A later enabled call then closes stale state and emits an invalid duration.
## Issue Context
`profile()` is a paired-call API, while the daemon can switch the shared legacy logger and level around requests. Disabled calls should not format messages or record timestamps, but they must not preserve an earlier open interval.
## Fix Focus Areas
- components/legacy/logger/logger.ts[209-218]
- components/legacy/logger/profiler.ts[18-35]
- scopes/harmony/api-server/cli-raw.route.ts[100-110]
- scopes/harmony/api-server/cli-raw.route.ts[169-171]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Results up to commit 21fbd30

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Level changes corrupt profiles 🐞 Bug ◔ Observability

Description

The new disabled-level return skips the closing state transition for an already-open profile, so if
the logger level changes between paired calls, the next enabled call closes the stale measurement
and reports time across unrelated work. This is reachable because the daemon swaps the global legacy
logger and its level around CLI requests.

Code

components/legacy/logger/logger.ts[214]
+    if (!shouldWriteToConsole && !this.logger.isLevelEnabled(level)) return;

Evidence
The documented API opens and closes a measurement with two calls, and Profiler removes current
only when another call reaches its closing branch. The changed return prevents that branch from
running, while the daemon route changes the shared logger before a command and restores it
afterward, making level changes during the profiler's lifetime a supported runtime behavior.
components/legacy/logger/logger.ts[194-218]
components/legacy/logger/profiler.ts[18-35]
scopes/harmony/api-server/cli-raw.route.ts[100-110]
scopes/harmony/api-server/cli-raw.route.ts[145-171]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A profile opened while its level is enabled remains open if its matching call occurs after that level is disabled. A later enabled call then closes stale state and emits an invalid duration.

## Issue Context
`profile()` is a paired-call API, while the daemon can switch the shared legacy logger and level around requests. Disabled calls should not format messages or record timestamps, but they must not preserve an earlier open interval.

## Fix Focus Areas
- components/legacy/logger/logger.ts[209-218]
- components/legacy/logger/profiler.ts[18-35]
- scopes/harmony/api-server/cli-raw.route.ts[100-110]
- scopes/harmony/api-server/cli-raw.route.ts[169-171]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

…s cap

qodo-free-for-open-source-projects · 2026-08-31T20:27:37Z

Code review by qodo was updated up to the latest commit 09b41e0

…r id

qodo-free-for-open-source-projects · 2026-08-31T20:33:16Z

Code review by qodo was updated up to the latest commit 98c3232

fix(logger): skip profiling when the level is off, bound the profiler…

21fbd30

…s map

qodo-free-for-open-source-projects Bot reviewed Aug 31, 2026

View reviewed changes

Comment thread components/legacy/logger/logger.ts Outdated

fix(logger): reset open profiles on logger switch, raise the profiler…

09b41e0

…s cap

qodo-free-for-open-source-projects Bot reviewed Aug 31, 2026

View reviewed changes

Comment thread components/legacy/logger/profiler.ts Outdated

Comment thread components/legacy/logger/logger.ts Outdated

GiladShoham approved these changes Aug 31, 2026

View reviewed changes

fix(logger): separate running from completed measurements, discard pe…

98c3232

…r id

davidfirst changed the title ~~fix(logger): skip profiling when the level is off, bound the profilers map~~ Aug 31, 2026

Navigation Menu

Uh oh!

fix(logger): skip profiling when the level is off, bound the profiler state - #10677

fix(logger): skip profiling when the level is off, bound the profiler state#10677
davidfirst wants to merge 3 commits into
masterfrom
fix-logger-profiler-overhead

davidfirst commented Aug 31, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Aug 31, 2026

qodo-free-for-open-source-projects Bot commented Aug 31, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 31, 2026

qodo-free-for-open-source-projects Bot commented Aug 31, 2026

Labels

2 participants

Uh oh!

Conversation

davidfirst commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 31, 2026

PR Summary by Qodo

qodo-free-for-open-source-projects Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 31, 2026

qodo-free-for-open-source-projects Bot commented Aug 31, 2026

Labels

2 participants

davidfirst commented Aug 31, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Aug 31, 2026 •

edited

Loading