runner: report high memory usage #1943
Conversation
packages/runner/lib/server.ts
Outdated
| @@ -5,6 +5,7 @@ import type { Request, Response, NextFunction } from 'express'; | |||
| import timeout from 'connect-timeout'; | |||
| import type { NangoProps, RunnerOutput } from '@nangohq/shared'; | |||
| import { getLogger } from '@nangohq/utils/dist/logger.js'; | |||
| import * as Usage from './usage.js'; | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
TBonnin
force-pushed
the
tbonnin/nan-665/runner-report-oom
branch
4 times, most recently
from
01b97c3 to
b9f6a71
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
TBonnin
force-pushed
the
tbonnin/nan-665/runner-report-oom
branch
from
b9f6a71 to
9d3fbf3
Compare
TBonnin
force-pushed
the
tbonnin/nan-665/runner-report-oom
branch
5 times, most recently
from
41b77c7 to
cddbc26
Compare
|
@khaliqgant @bodinsamuel thank you for your feedback. I have refactored the PR quite a bit. Would you mind giving it another look? |
| private idleMaxDurationMs = parseInt(process.env['IDLE_MAX_DURATION_MS'] || '') || 0; | ||
| private lastIdleTrackingDate = Date.now(); | ||
| private lastMemoryReportDate: Date | null = null; | ||
| private idleInterval: NodeJS.Timeout | null = null; |
There was a problem hiding this comment.
a bit out of scope but I renamed this entire logic RunnerMonitor and moved the idle logic into it, since it consist in monitoring the runner idling time
| // see: https://github.com/nodejs/node/issues/51095 | ||
| // process.constrainedMemory() is supposed to return the memory limit of the container but it doesn't work on Render | ||
| // so we need to use a workaround to get the memory limit of the container on Render | ||
| return process.constrainedMemory() || getRenderTotalMemoryInBytes() || os.totalmem(); |
There was a problem hiding this comment.
This is incredibly fragile but I don't have a better solution to make it work in Render since os.totalmem doesn't return the container memory limit but the vm's one :( Let me know what you think
TBonnin
force-pushed
the
tbonnin/nan-665/runner-report-oom
branch
from
cddbc26 to
4c230ba
Compare
TBonnin
force-pushed
the
tbonnin/nan-665/runner-report-oom
branch
3 times, most recently
from
3731fc8 to
0a27a58
Compare
There was a problem hiding this comment.
Looks very nice 🥰 sorry for all the feedbacks and thanks for the patience!
I have an other, running locally I get this error.
[2] logger.warn(`Error reading memory limit from ${memoryMaxFile}: ${stringifyError(error)}`);
[2] ^
[2]
[2] TypeError: logger.warn is not a function
[2] at getRenderTotalMemoryInBytes (file:///Users/samuelbodin/code/nango/packages/runner/dist/monitor.js:114:16)
[2] at getTotalMemoryInBytes (file:///Users/samuelbodin/code/nango/packages/runner/dist/monitor.js:123:43)
[2] at Timeout._onTimeout (file:///Users/samuelbodin/code/nango/packages/runner/dist/monitor.js:52:27)Raises two questions why it throwed and why logger do not have warn 🤔
edit: I have checked, we have configured the logger with syslog.levels which has warning not warn. Type is not good unfortunately.
Might not exists in macOs
syscall: 'open',
[2] code: 'ENOENT',
[2] path: '/sys/fs/cgroup/memory.max'
TBonnin
force-pushed
the
tbonnin/nan-665/runner-report-oom
branch
from
0a27a58 to
fdaf59c
Compare
|
@bodinsamuel I have decided to remove the logging. It is super noisy when running locally |
TBonnin
force-pushed
the
tbonnin/nan-665/runner-report-oom
branch
from
fdaf59c to
8371a79
Compare
Describe your changes
The goal is to report the runner high memory usage so users can understand better when a script is failing.
This is not a perfect solution, for example if memory spikes and runner crashes within a check interval no activity log would be emitted.
@bastienbeurier this is how it looks like
Issue ticket number and link
https://linear.app/nango/issue/NAN-665/make-runner-out-of-memory-error-more-explicit
Checklist before requesting a review (skip if just adding/editing APIs & templates)