Repository navigation
Conversation
|
@jamal-exe Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Miracle656
left a comment
There was a problem hiding this comment.
The instinct is right — the ingestion failures genuinely are unqueryable today, and the fields you chose (venue, network, pair, attempt, retryMs, durationMs) are the ones an operator actually wants. Capturing currentPair so a cycle failure says which pair it died on is a nice touch that the original console.error could not do.
But this adds a third logging convention, and there is already a PR replacing the first two.
src/logging.ts here is a hand-rolled JSON.stringify over console.*. #402 is open against #278 and routes everything through pino, which Fastify already ships and already attaches to every request as req.log — component-scoped children (ingester:sdex, ws:fanout, route:price), a configurable LOG_LEVEL, field scrubbing, and Fastify's request logger built from the same options so server logs and application logs are formatted identically.
If both land, the repo has console.*, logEvent() and logger.info() all in use, and docs/logging.md documenting the one that is not the house standard. That is worse than where we started, because at least console.* was consistently wrong.
Two specific things pino gives you that logEvent cannot, which is why the issue asked for it:
- A level you can turn down.
logEvent('error', …)always writes. There is no way to quiet a noisy ingester in production without a deploy. - No timestamp. The record is
{level, message, ...fields}— nothing orders these events. For a retry loop loggingattemptandretryMs, when it happened is most of the value.
What I would like
Rebase onto #402 once it lands — I have asked for a rebase on it and it is next in my queue — and re-express this as pino calls on an ingester:<venue> child. Your field set is the good part and should survive; it is only the transport that should change.
logger.error({ venue: adapter.name, network, pair: currentPair, attempt: consecutiveErrors, retryMs: backoffMs, durationMs, err }, 'venue ingestion cycle failed')Pass the error as err rather than error: (err as Error).message — pino serialises it with its stack, and the stack is what you want at 3am.
docs/logging.md should then document the pino convention rather than this one. If you would rather fold that documentation into #402 and close this, that is fine too — say which you prefer and I will work with it. I am not going to close your PR either way; it is your call.
One last thing, unrelated to the transport: #396 (yours too) touches runner.ts and amm.ts as well, and I have already asked for changes there. Whichever you push first, the other needs a rebase.
Adds a shared JSON logging helper for ingestion failures, structured venue/network/pair/status fields, and a Loki query example for filtering failures by pair. Background ingestion does not currently have request IDs, so none are fabricated.
Closes #278