fix: optimized how we receive the package.json information #2176
Conversation
aryamohanan
force-pushed
the
fix-pkg-resolution
branch
2 times, most recently
from
6512b35 to
54af1ea
Compare
kirrg001
changed the title
kirrg001
force-pushed
the
fix-pkg-resolution
branch
from
a945e25 to
48a0261
Compare
| metricsModules.forEach(metricsModule => { | ||
| if (metricsModule.activate) { | ||
| metricsModule.activate(config); | ||
| util.applicationUnderMonitoring.getMainPackageJsonStartingAtMainModule((err, packageJsonObj) => { |
There was a problem hiding this comment.
There was a bigger problem in the codebase:
Any metric requested the pkg json (in parallel).
This created too many log entries and also its logically not right.
Any outer pkg calls core.metrics.activate, which activates all registered metrics.
We request the pkg json here once and then forward a payload to the metric.
| util.applicationUnderMonitoring.getMainPackageJsonStartingAtMainModule((err, packageJsonObj) => { | ||
| if (err) { | ||
| logger.warn( | ||
| `Failed to determine main package.json. Some metrics might not work. Reason: ${err?.message} ${err?.stack}` |
There was a problem hiding this comment.
Log once. And also mention that only some metrics might not work.
|
|
||
| const MAX_ATTEMPTS = 5; | ||
| const DELAY = 1500; | ||
| let attempts = 0; |
There was a problem hiding this comment.
Centralized retries (see name.js and version.js etc.).
There is no need to retry from all the single metrics.
We still need the retries because of using require/import as arguments (see comment in line 163!)
| // NOTE: we already cached the package.json | ||
| if (parsedMainPackageJson !== undefined) { | ||
| return cb(null, parsedMainPackageJson); | ||
| return cb(null, { file: parsedMainPackageJson, path: mainPackageJsonPath }); |
There was a problem hiding this comment.
Always return both file + path. Some metrics need the file, some the path.
| * @param {(err: Error, packageJsonPath: string) => void} cb - the callback will be called with an error or the path to | ||
| * the package.json file | ||
| */ | ||
| function getMainPackageJsonPathStartingAtMainModule(cb) { |
There was a problem hiding this comment.
Not needed anymore, because we now request pkg json + pkg json path centralized in core metrics index.js. We always provide the found path and the file to the metric.
| // CASE: node --require .../src/immediate.js | ||
| if (!mainModule?.filename) { | ||
| const err = new Error('Application entrypoint could not be identified.'); | ||
| return process.nextTick(cb, err, null); |
There was a problem hiding this comment.
This is the actual fix. @aryamohanan started with it already.
We now also forward the error.
| // Install the shared metrics module | ||
| const sharedMetricsPath = path.join(__dirname, '..', '..', '..', 'shared-metrics'); | ||
| runCommandSync('npm pack', sharedMetricsPath); | ||
| execSync('./preinstall.sh', { cwd: __dirname, stdio: 'inherit' }); |
There was a problem hiding this comment.
Will outsource to main. Test was failing because shared-metrics was not installed from local folder - it took the version from NPM - this does not work if you do refactorings (such as removing fns)
| execSync('npm install --no-save --no-package-lock --prefix ./ ./shared-metrics.tgz', { | ||
| cwd: __dirname, | ||
| stdio: 'inherit' | ||
| }); |
There was a problem hiding this comment.
Will outsource to main. Test was failing because shared-metrics was not installed from local folder - it took the version from NPM - this does not work if you do refactorings (such as removing fns)
kirrg001
changed the title
refs https://jsw.ibm.com/browse/INSTA-65637
Problem:
Before:
After:
