Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: guard against apm-clients encoding/truncation of trace objects throwing #4359

Merged
merged 2 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ See the <<upgrade-to-v4>> guide.
[float]
===== Bug fixes

* Guard against a possible encoding error of tracing data in the APM client,
before it is sent. It is *possible* this could wedge the APM client,
resulting in the APM agent no longer sending tracing data.
({pull}4359[#4359])

[float]
===== Chores

Expand Down
73 changes: 63 additions & 10 deletions lib/apm-client/http-apm-client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,15 @@ Client.prototype._resetEncodedMetadata = function () {

// This is the only code path that should set `_encodedMetadata`.
this._encodedMetadata = this._encode({ metadata }, Client.encoding.METADATA);
if (!this._encodedMetadata) {
// The APM client cannot function without encoded metadata. Handling this
// could be improved (e.g. log details and disable the APM agent). However,
// this suffices for now as we have never observed a metadata encoding
// failure.
throw new Error(
'could not encode metadata (trace-level logging will include details)',
);
}
this._log.trace(
{ _encodedMetadata: this._encodedMetadata },
'_resetEncodedMetadata',
Expand Down Expand Up @@ -504,6 +513,9 @@ Client.prototype._write = function (obj, enc, cb) {
} else {
const t = process.hrtime();
const chunk = this._encode(obj, enc);
if (!chunk) {
return;
}
this._numEventsEnqueued++;
this._chopper.write(chunk, cb);
this._log.trace(
Expand Down Expand Up @@ -579,12 +591,18 @@ Client.prototype._writeBatch = function (objs, cb) {
const chunks = [];
for (var i = 0; i < objs.length; i++) {
const obj = objs[i];
chunks.push(this._encode(obj.chunk, obj.encoding));
const encoded = this._encode(obj.chunk, obj.encoding);
if (encoded) {
chunks.push(encoded);
}
}
if (chunks.length === 0) {
return;
}
const chunk = chunks.join('');
const encodeTimeMs = deltaMs(t);

this._numEventsEnqueued += objs.length;
this._numEventsEnqueued += chunks.length;
this._chopper.write(chunk, cb);
const fullTimeMs = deltaMs(t);

Expand All @@ -601,7 +619,7 @@ Client.prototype._writeBatch = function (objs, cb) {
{
encodeTimeMs,
fullTimeMs,
numEvents: objs.length,
numEvents: chunks.length,
numBytes: chunk.length,
},
'_writeBatch',
Expand Down Expand Up @@ -687,24 +705,54 @@ Client.prototype._maybeUncork = function () {
};

Client.prototype._encode = function (obj, enc) {
const out = {};
let thing;
let truncFunc;
let outAttr;
switch (enc) {
case Client.encoding.SPAN:
out.span = truncate.span(obj.span, this._conf);
thing = obj.span;
truncFunc = truncate.span;
outAttr = 'span';
break;
case Client.encoding.TRANSACTION:
out.transaction = truncate.transaction(obj.transaction, this._conf);
thing = obj.transaction;
truncFunc = truncate.transaction;
outAttr = 'transaction';
break;
case Client.encoding.METADATA:
out.metadata = truncate.metadata(obj.metadata, this._conf);
thing = obj.metadata;
truncFunc = truncate.metadata;
outAttr = 'metadata';
break;
case Client.encoding.ERROR:
out.error = truncate.error(obj.error, this._conf);
thing = obj.error;
truncFunc = truncate.error;
outAttr = 'error';
break;
case Client.encoding.METRICSET:
out.metricset = truncate.metricset(obj.metricset, this._conf);
thing = obj.metricset;
truncFunc = truncate.metricset;
outAttr = 'metricset';
break;
}

const out = {};
try {
out[outAttr] = truncFunc(thing, this._conf);
} catch (err) {
this._log.warn(
{
err,
// Only log full problematic object at TRACE level to limit noise.
thing: this._log.isLevelEnabled('trace') ? thing : '[REDACTED]',
thing_id: thing?.id,
thing_name: thing?.name,
},
`could not encode "${outAttr}" object`,
);
return null;
}

return ndjson.serialize(out);
};

Expand Down Expand Up @@ -765,7 +813,6 @@ Client.prototype.lambdaRegisterTransaction = function (trans, awsRequestId) {
{ awsRequestId, traceId: trans.trace_id, transId: trans.id },
'lambdaRegisterTransaction start',
);
var out = this._encode({ transaction: trans }, Client.encoding.TRANSACTION);

const finish = (errOrErrMsg) => {
const durationMs = performance.now() - startTime;
Expand All @@ -784,6 +831,12 @@ Client.prototype.lambdaRegisterTransaction = function (trans, awsRequestId) {
resolve(); // always resolve, never reject
};

var out = this._encode({ transaction: trans }, Client.encoding.TRANSACTION);
if (!out) {
finish('could not encode transaction');
return;
}

// Every `POST /register/transaction` request must set the
// `x-elastic-aws-request-id` header. Instead of creating a new options obj
// each time, we just modify in-place.
Expand Down
Loading