feat: Collect OTEL metrics in AWS Lambda - #4166
Conversation
Add coverage for the serverless OTEL metrics path introduced on this branch and fix an audit-logging regression it surfaced. - Fix `NRProxyingSerializer` audit guard: `auditEnabled === true` compared the function reference (always false); call it instead. Caught by the existing "logs when serializing" test. - Repair broken tests the private-field/serverless refactor left red: rewrite `#flushPayloadSync` to assert on real flushed output rather than monkeypatching the now-private `#doFlush`, and give the metrics index mock agent a `serverlessMode` so both constructor branches are reachable. - Add coverage for `lastSerialization` caching/purge and `awsLambdaMode`, the `serializedData` getter, the serverless no-`started`-listener path, `flushToString` (isolated file: global meter provider + network), and `ServerlessCollector` attaching `otlp_payload` from `flushToString`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
This object has a few methods it only uses internally. I found it difficult to determine which methods actually needed to be exposed for the agent to work correctly. So I updated the internal only methods to be private. This makes it clear which methods are meant to be the public API and which are meant to be private.
There was a problem hiding this comment.
As a consequence of making the API clearer, tests needed to be updated such that they monkey patch the underlying filesystem methods instead of replacing methods directly on the collector.
| assert.equal(flushed, true) | ||
| }) | ||
|
|
||
| await t.test('includes OTEL metrics as otlp_payload when the metrics API is present', async (t) => { |
There was a problem hiding this comment.
This is a new test that verifies the collector attaches the serialized protobuf data correctly.
Add debug logs distinguishing the standard (defer to agent connect) and serverless (finalize eagerly) OTEL metrics bootstrap paths, and cover them in the metrics setup tests via a capturing logger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4166 +/- ##
==========================================
- Coverage 97.51% 97.44% -0.08%
==========================================
Files 517 520 +3
Lines 62593 62778 +185
Branches 1 1
==========================================
+ Hits 61039 61175 +136
- Misses 1554 1603 +49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Follow the refactor to an in-memory capturing exporter: - Fix flushToString to read `lastSerialization` (NRCapturingExporter's accessor); it was still reading the removed `serializedData` getter and returning undefined. - Add nr-capturing-exporter unit tests: serialize + SUCCESS callback, audit log, cache purge-on-read, and forceFlush/shutdown no-ops. - Drop the now-obsolete network stubbing from the flushToString test (the capturing exporter does no network I/O) and assert serverless bootstrap emits `otelMetricsBootstrapped` synchronously. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bizob2828
left a comment
bizob2828
left a comment
There was a problem hiding this comment.
A few comments/questions. I assumed you verified this with real lambdas?
| // be fully synchronous here. That API is _both_ callback and promise | ||
| // based. So we have to finalize our export out-of-band when the metrics | ||
| // API is available (i.e. OTEL + OTEL Metrics have been enabled). | ||
| this._agent.otel.metrics.flushToString().then((serializedMetrics) => { |
There was a problem hiding this comment.
what happens if this fails? There's no catch handler so the agent may produce an unhandledRejection
There was a problem hiding this comment.
Also are we certain this is actually going to work? this happens right before the lambda shutdown, feels like there's a chance of data loss because the promise may not have resolved
There was a problem hiding this comment.
It's not really possible for it to fail.
The promise comes from the .collect method on the metrics reader object. That method is just compiling a JSON object of all the data that has been buffered.
The callback comes from the .export method. Since we supply our own exporter that only deals with the in-memory data, there's nothing for the callback to fail on.
Regarding shutdown, I'm open to suggestions. The OTEL API requires the codes as I have written it. Our code expects fully synchronous operations. So I can't await the promise but I have to use the promise. 🤷♂️
| return | ||
| } | ||
|
|
||
| if (this._agent.otel?.metrics?.flushToString) { |
There was a problem hiding this comment.
Feels weird to be checking if there's a property on otel.metrics. What if there's some idea of enabled flags for all these otel apis?
There was a problem hiding this comment.
Example please?
| // proxy so that we can manually collect metrics when the agent performs | ||
| // a harvest during serverless mode execution. | ||
| logger.debug('Finalizing OTEL metrics in serverless mode.') | ||
| proxyExporter.exporter = new NRCapturingExporter({ logger }) |
There was a problem hiding this comment.
since we do not do a round trip with collector it's missing the entity_guid and any other otlp_resource_attibutes. Is the extension aware they will need to decorate metrics with those attributes?
There was a problem hiding this comment.
That's a question for the team implementing the extension. To my knowledge, it already does this.
| async flushToString() { | ||
| const { resourceMetrics: collectedMetrics } = await this.#metricReader.collect() | ||
| await new Promise((resolve) => { | ||
| this.#metricExporter.exporter.export(collectedMetrics, () => { |
There was a problem hiding this comment.
can you provide a comment explaining this. so we collect the metrics, then call export but return what was last serialized? What happens if the export fails, could it return what was last collected on warm lambda invocations?
There was a problem hiding this comment.
What suggestion do you have for the jsdoc block? I think it already provides the explanation.
There isn't any failure case possibility here that I am aware of. As explained above, all of this is happening on in-memory data. The only failure case I know of for .export is it not being able to ship the collected data to a remote HTTP endpoint. We are avoiding that completely.
| const generateProxyAgentFactory = require('./generate-proxy-agent-factory.js') | ||
|
|
||
| // See https://developer.mozilla.org/en-US/docs/Web/API/Window/setTimeout#maximum_delay_value | ||
| const MAXIMUM_TIMEOUT_TIME = ((2 ^ 32) / 2) - 1 |
There was a problem hiding this comment.
do you have to provide an export interval? Doesn't this just create a timer that will never resolve and thus hang the process?
There was a problem hiding this comment.
Yes, we have to provide an interval. Otherwise we have to write our own metrics reader. The only actual implementation upstream provides is the periodic reader (https://open-telemetry.github.io/opentelemetry-js/classes/_opentelemetry_sdk-node.metrics.PeriodicExportingMetricReader.html).
The timer will not keep the process alive:
I verified this with AWS's Docker based Lambda environment. |
bizob2828
left a comment
bizob2828
left a comment
There was a problem hiding this comment.
We talked, I'm ok with this code, if there is an issue we can always address later. especially since this isn't 100% wired up with the lambda extension and this feature isn't GA
github-project-automation
Bot
moved this from Needs PR Review
to Done: Issues recently completed
in Node.js Engineering Board
3 participants
See newrelic/newrelic-node-examples#379.
We should probably just ship this as "prep for an upcoming feature" instead of keeping a stale branch around. The agent is going to do the right thing, but the data won't get shipped until the extension is updated (possibly in a future quarter).
Summary
Adds collection of OpenTelemetry metrics in the AWS Lambda (serverless) environment. In serverless mode the periodic exporter is effectively disabled and metrics are instead flushed on demand during the agent's synchronous harvest:
SetupMetrics.flushToString()collects and serializes the metrics to a base64 OTLP protobuf payload, whichServerlessCollectorattaches to the flushed payload asotlp_payload.Closes #4126.
Changes
SetupMetrics: in serverless mode, use a never-firing export interval, finalize the exporter eagerly (nostarted-event wait), and exposeflushToString().NRProxyingSerializer/NROTLPMetricExporter: cache the last serialization (lastSerialization/serializedData) so the harvest can retrieve the encoded payload after an export;awsLambdaModeforces caching independent of audit logging.ServerlessCollector.flushPayloadSync: pull OTEL metrics viaagent.otel.metrics.flushToString()when present and include them asotlp_payload; made internal flush helpers private.Agent: internalotelAPI accessor (attached bylib/otel/setup.js).Testing
NRProxyingSerializercompared theauditEnabledfunction reference totrueinstead of calling it.awsLambdaMode, theserializedDatagetter, the serverless no-started-listener path,flushToString(in its own file — it registers a process-global meter provider and reaches the network, nock-stubbed), andServerlessCollectorattachingotlp_payload.🤖 Generated with Claude Code