feat: add payloadLogFile debug option #9
Conversation
|
@Qard The codecov tests are failing. I don't think it's that relevant to add tests for this debug option, but I can do it if you like. Otherwise we'll either just merge or add a codecov.yml file to lower the failure limits. What do you think? |
README.md
Outdated
| ## Debugging | ||
|
|
||
| To see what data is being sent to the APM Server, use the environment | ||
| variable `ELASTIC_APM_PAYLOAD_LOG` to speicfy a log file, e.g: |
|
Any reason not to just do this as a regular config option, like everything else? |
|
@Qard hmm maybe not... I was just doing it like this because this is how it was done previously with the |
|
My thinking was that:
I'm good with this PR as-is, if you feel it's better to just have the environment variable reading here, rather than as a config input. |
watson
changed the title
|
@Qard good points... I converted it into a config option, refactored it a bit to include the metadata, added tests, and rebased with the base branch. So had to force push |
index.js
Outdated
| if (!client._payloadLogFile) { | ||
| client._payloadLogFile = require('fs').createWriteStream(opts.payloadLogFile, {flags: 'a'}) | ||
| } | ||
| pump(stream, client._payloadLogFile) |
There was a problem hiding this comment.
@Qard hmm pump might actually close the file once stream ends. I'll just investigate if this works for more than one request
index.js
Outdated
| @@ -220,6 +220,15 @@ function onStream (opts, client, onerror) { | |||
| next() | |||
| }) | |||
|
|
|||
| if (opts.payloadLogFile) { | |||
| if (!client._payloadLogFile) { | |||
| client._payloadLogFile = require('fs').createWriteStream(opts.payloadLogFile, {flags: 'a'}) | |||
There was a problem hiding this comment.
@Qard This will leave the file open forever. We could also just close it every time stream closed and then reopen it again at the next request. Is there a downside of just leaving it open like this?
There was a problem hiding this comment.
Since it's intended just for debugging purposes, I don't think it matters much. I think it's fine to leave it as-is.
index.js
Outdated
| } | ||
| stream.on('data', function (chunk) { | ||
| client._payloadLogFile.write(chunk) | ||
| }) |
There was a problem hiding this comment.
Is there a reason you don't use pipe/pump?
There was a problem hiding this comment.
If I used pipe/pump it would close the file when stream ended. That's not an issue as such, but then I couldn't re-use the file handle and would have to open it again for each stream that was opened
There was a problem hiding this comment.
But maybe that's cleaner? Even though there's an overhead of re-opening the file for each stream. As it's just for debug purposes, I'm not that concerned with the performance.
There was a problem hiding this comment.
Didn't think about the closing issue. In that case, we just leave it as-is.
No description provided.