telemetry: slog sender with block chunking #10710
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
6e1ebde
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5bd8a177.agoric-sdk.pages.dev |
| Branch Preview URL: | https://usman-block-slogger.agoric-sdk.pages.dev |
|
|
||
| const stream = handle | ||
| ? createWriteStream(noPath, { fd: handle.fd }) | ||
| ? createWriteStream(noPath, { autoClose: false, fd: handle.fd }) |
There was a problem hiding this comment.
Why set autoClose: false ? Afaik we don't sufficiently monitor stream error/finish to correctly call close on it.
| stream.close?.(); | ||
|
|
||
| if (handle) { | ||
| await new Promise(resolve => stream.end(() => resolve(null))); |
There was a problem hiding this comment.
Switching from close to end with autoClose: false will result in the stream not getting destroyed. Is that intended? I suppose we explicitly close the handle below.
There was a problem hiding this comment.
I'm not sure what motivates the changes here, but if we're gonna change this, I'm wondering if it might not be better to use the "new" filehandle.createWriteStream API that exists since v16.11.
Also depending on the reasoning for no longer auto-closing the handle, we likely want to to use the new flush option.
| */ | ||
| export const makeSlogSender = async options => { | ||
| const { CHAIN_ID, CONTEXTUAL_BLOCK_SLOGS } = options.env || {}; | ||
| if (!(options.stateDir || CONTEXTUAL_BLOCK_SLOGS)) |
There was a problem hiding this comment.
If both are needed, this condition isn't correct
| if (!(options.stateDir || CONTEXTUAL_BLOCK_SLOGS)) | |
| if (!options.stateDir || !CONTEXTUAL_BLOCK_SLOGS) |
There was a problem hiding this comment.
No, atleast one of them is needed
| { | ||
| 'chain-id': CHAIN_ID, | ||
| }, | ||
| persistenceUtils, |
There was a problem hiding this comment.
Were we not passing the persistence utils for the file version? I totally missed that. Let's extract this fix in a separate PR.
packages/telemetry/src/block-slog.js
Outdated
| const contextualSlogProcessor = makeContextualSlogProcessor( | ||
| { 'chain-id': CHAIN_ID }, | ||
| persistenceUtils, | ||
| ); |
There was a problem hiding this comment.
Why are we using a contextual slog? I'm not sure we care about contextualizing, as the goal of this tool is mostly for archiving, not as much for querying. Most tools we have currently work against original slogs events.
packages/telemetry/src/block-slog.js
Outdated
| /** | ||
| * @param {import('./context-aware-slog.js').Slog} slog | ||
| */ | ||
| const slogSender = async slog => { |
There was a problem hiding this comment.
Slog senders cannot be async. Please use an internal queue if an async implementation is actually needed (and make sure that forceFlush respects this queue. The flight recorder has a trivial example
I'm also not a fan of all the console.error logging on errors. I think we need to let errors go back up to the result promise, and for flush to be able to report an aggregation of all errors. An error happening for one event should not prevent us from attempting to write another event.
|
@mhofman I have reverted other changes. I will fix the absent |
usmanmani1122
changed the title
closes: #10779
refs: #xxxx
Description
This PR adds a slog sender with block level slog files. There is also builtin compression support. The slog files generated will be of the following format:
Where identifier could be on of the following:
initbootstrapupgradeblock_${blockHeight}Security Considerations
None
Scaling Considerations
None
Documentation Considerations
None
Testing Considerations
None
Upgrade Considerations
None