Skip to content

Conversation

@justinmk3
Copy link
Contributor

Problem

"memory threshold" message logged multiple times for a the same PID:

2025-04-11 ... Process 23662 exceeded memory threshold: 506986496
2025-04-11 ... Process 23662 exceeded memory threshold: 507019264
2025-04-11 ... Process 23662 exceeded memory threshold: 507052032
2025-04-11 ... Process 23662 exceeded memory threshold: 507084800

This is noisy in the logs.

Solution

Only log "memory threshold" once per PID.


  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

Problem:
"memory threshold" message logged multiple times for a the same PID:

    2025-04-11 ... Process 23662 exceeded memory threshold: 506986496
    2025-04-11 ... Process 23662 exceeded memory threshold: 507019264
    2025-04-11 ... Process 23662 exceeded memory threshold: 507052032
    2025-04-11 ... Process 23662 exceeded memory threshold: 507084800

This is noisy in the logs.

Solution:
Only log "memory threshold" once per PID.
@justinmk3 justinmk3 requested a review from a team as a code owner April 11, 2025 15:15
@github-actions
Copy link

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.
  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

cpu: 50,
}
static readonly logger = logger.getLogger('childProcess')
static readonly #loggedPids = new CircularBuffer(1000)
Copy link
Contributor

@Hweinstock Hweinstock Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we concerned about the case where a child process has a problem (ex. memory leak) that causes it to increase its resource usage as time passes? This change would make it hard to catch that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a valid concern, but:

  • by this point the "threshold" was already exceeded, which means we presumably already reached an alarm-state
  • to give visibility into "growing"/leak behavior, we should enhance the logic rather than hoping that someone will puzzle it out through these logs

@Hweinstock
Copy link
Contributor

This test

it('logs a warning message when system usage exceeds threshold', async function () {
const runningProcess = startSleepProcess()
tracker.add(runningProcess.childProcess)
const highCpu: ProcessStats = {
cpu: ChildProcessTracker.thresholds.cpu + 1,
memory: 0,
}
const highMemory: ProcessStats = {
cpu: 0,
memory: ChildProcessTracker.thresholds.memory + 1,
}
usageMock.returns(highCpu)
await clock.tickAsync(ChildProcessTracker.pollingInterval)
assertLogsContain('exceeded cpu threshold', false, 'warn')
usageMock.returns(highMemory)
await clock.tickAsync(ChildProcessTracker.pollingInterval)
assertLogsContain('exceeded memory threshold', false, 'warn')
await stopAndWait(runningProcess)
})
could be adjusted to use two different processes for the logs messages or perhaps split into two tests, one for each threshold, and that should fix CI.

Otherwise LGTM!

@justinmk3
Copy link
Contributor Author

use two different processes for the logs messages

That's probably better. For now I just clear() the circularbuffer in beforeEach.

@justinmk3
Copy link
Contributor Author

/runintegrationtests

@justinmk3 justinmk3 merged commit 4c0d098 into aws:master Apr 11, 2025
34 of 37 checks passed
@justinmk3 justinmk3 deleted the fixqlogin branch April 11, 2025 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants