feat(instr-runtime-node): add configurable gcDurationBuckets option

[lukeramsden]

Which problem is this PR solving?

Monitoring precision being configurable but not the bucket boundary for GC duration seems like an oversight in this package. I'd like to add 2s and 5s boundaries in my application.

Short description of the changes

Allow users to customize the histogram bucket boundaries for the v8js.gc.duration metric via the gcDurationBuckets configuration option, similar to how monitoringPrecision is configurable.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: lukeramsden / name: Luke Ramsden (b0c20ee, c627f76)

[maryliag]

Thank you for your contribution.
You will need to add a changelog and tests to this PR before we can merge it.

[trentm]

You will need to add a changelog and tests to this PR before we can merge it.

@maryliag This gets me and I have to check again almost everytime. The core repo requires PRs to explicitly include a CHANGELOG.md entry. The contrib repo does not, because the release process automatically adds changelog entries based on the commit titles.

[maryliag]

The contrib repo does not

this is what I get for having a bunch of tabs open to review and not checking the repo 🤦
Now just wait for tests to pass then 😄

[lukeramsden]

I have pushed some tests for the GC collection, which pass locally. Will leave the changelog alone then 😄

[lukeramsden]

I've pushed a fix for prettier lint but the test failures don't make sense to me - no test files found?

[maryliag]

the test failures might be related to this change, what this parameter does? does it need some extra info to find specific files?

[lukeramsden]

This exposes the ability to trigger GC which is what the test uses.

[lukeramsden]

I don't see how it would be possible to test this instrumentation collector without this flag which is I assume why there were no tests in the first place

I believe that v18 and v20 of Node that your CI tests with do not have this flag and therefore I do not know how one would test this collector on those versions. Up to you how to proceed, we could:

1. Not test this collector (which is the current state of the codebase and my change does not make particularly any worse) and therefore delete my tests
2. Disable these tests in certain CI matrix formations where we use older Node.js versions
3. Find an alternative way to do this on those versions, however I do not know of a way to do that

[david-luna]

There is a script that can be used to skip tests given a certain major version (only major). A possible solution is to:

• have a test script without the flag (works for all versions)
• have a test script with the flag and skip for nodejs versions lower than 22
• refactor the new tests to skip if the global.gc() method is not avaiable

Example of how the scripts would look like.

    "test": "npm run test:nogc && npm run test:gc",
    "test:nogc": "nyc --no-clean mocha 'test/**/*.test.ts'",
    "test:gc": "SKIP_TEST_IF_NODE_OLDER_THAN=22 nyc --no-clean mocha --require '../../scripts/skip-test-if.js' --expose-gc 'test/**/*.ts'",

WDYT?

[pichlermarc]

Silly question: is this not what the View concept is for (defining a custom aggregation based on meter+metric name?)
https://opentelemetry.io/docs/languages/js/instrumentation/#configure-metric-views

[lukeramsden]

Silly question: is this not what the View concept is for (defining a custom aggregation based on meter+metric name?) https://opentelemetry.io/docs/languages/js/instrumentation/#configure-metric-views

Does that work if the view buckets are more granular or have larger ranges than the underlying histogram? I've never heard of views and no idea how they work internally (and docs aren't clear)

[pichlermarc]

Silly question: is this not what the View concept is for (defining a custom aggregation based on meter+metric name?) https://opentelemetry.io/docs/languages/js/instrumentation/#configure-metric-views

Does that work if the view buckets are more granular or have larger ranges than the underlying histogram? I've never heard of views and no idea how they work internally (and docs aren't clear)

Yes. The underlying mechanism the same for View and advice - when you use Histogram#record() it passes the data to the Aggregation that was created based on either the default, the advice or the View. In any case, you'll not loose data-granularity and there's no estimation going on.

[david-luna]

@lukeramsden did you have time to review the latest comments?