feat: truncate payloads according to intake API limits #8
Conversation
README.md
Outdated
| Elasticsearch keywords. Strings larger than this will be trucated | ||
| (default: `1024` bytes) | ||
| - `truncateErrorMessagesAt` - The maximum size in bytes for error | ||
| messages. Messages above this length will be truncated. Set to `-1` do |
index.js
Outdated
| @@ -108,14 +108,17 @@ Client.prototype._write = function (obj, enc, cb) { | |||
| } | |||
|
|
|||
| Client.prototype.sendSpan = function (span, cb) { | |||
| truncate.span(span, this._opts) | |||
There was a problem hiding this comment.
I'd prefer we do this in the write method, to enable full functionality even when using directly as a stream.
index.js
Outdated
| @@ -228,7 +235,9 @@ function onStream (opts, client, onerror) { | |||
| }) | |||
|
|
|||
| // All requests to the APM Server must start with a metadata object | |||
| stream.write(safeStringify({metadata: metadata(opts)}) + '\n') | |||
| const metadata = getMetadata(opts) | |||
| truncate.metadata(metadata, opts) | |||
There was a problem hiding this comment.
You could do this truncation in the write method too.
There was a problem hiding this comment.
stream.write isn't the same write method. Here I write directly to the StreamChopper stream.
There was a problem hiding this comment.
Any reason not to just do this through the regular write? Maybe we don't want users sending their own metadata objects?
There was a problem hiding this comment.
My primary reason for writing directly to the internal stream was to skip having to go through an extra unnecessary step in the pipeline (performance). But it's definitely the idea that we want to handle writing the metadata on behalf of the user. The api is such that the user doesn't have to concern them selfs with when an HTTP request ends and another begins. So they shouldn't have to know when metadata is written either.
But I'm actually just about to submit a new PR that changes the internal pipeline structure, so if we hold off merging this for a short while it might all change anyway.
There was a problem hiding this comment.
Another and actually more important reason why I write directly to the internal request stream is that we want to make sure that the metadata is the first object in the new HTTP request. I'm not sure how I can easily guarantee this unless I write it this way 🤔
There was a problem hiding this comment.
The PR I referred to two comments ago, didn't make such a huge change after all, but as I said in my last comment, I don't think there's any reasonable way around doing it this way. But I might not fully understand the reason for wanting to write the metadata to the client stream instead of the http request stream?
There was a problem hiding this comment.
I was just thinking it might simplify things to have all the truncation logic in one place, but if sending the metadata at a request start is a concern, then it makes sense to keep it this way. 👍
index.js
Outdated
| truncate.span(obj, this._opts) | ||
| } else if ('error' in obj) { | ||
| truncate.error(obj, this._opts) | ||
| } |
There was a problem hiding this comment.
Should these not point to the sub-object, not the base object? ie: obj.transaction?
Also, is there a reason to else-if these? Are we unable to send different types at the same time?
There was a problem hiding this comment.
Oh you're right, it should be truncate.error(obj.error, this._opts) etc. I'll make an update.
The client is an object stream, so each write will be a single object containing either a transaction, a span, or an error. I could use a switch(Object.keys(obj)[0]), but I felt that was less performant than 3 if-statements.
There was a problem hiding this comment.
I can see a case for sending a span and error at the same time. You wrap an async call in a span from call to callback, and an error occurs, so you report the error and span when the callback is reached. 🤔
There was a problem hiding this comment.
Version 2 of the APM Server intake API currently doesn't support that an object contains more than a single root property - either transaction, span or error.
There was a problem hiding this comment.
Ah, that's fine then. Might be worth considering for @elastic/apm-server to support in the future though.
No description provided.