Repository navigation
feat(metrics): track venue ingestion health - #396
Conversation
|
@Herikcool 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 metric design here is right, and in a way that suggests you read the surrounding material rather than just the issue.
Labels are venue, network and a bounded category — all closed sets — and you left a comment telling the next person not to add pair or pool identifiers. That is the lesson from #337 applied before it bites rather than after. The histogram buckets are chosen for this service with a sentence explaining why, instead of prom-client's defaults.
Exposing ingest_last_success_timestamp as a Unix timestamp rather than an age is also the better choice, and not the one my issue asked for. A gauge holding "seconds since" is wrong the moment scraping stalls, because it keeps counting from a value nobody refreshed; time() - ingest_last_success_timestamp is computed at query time and is always honest. Good call.
Two things to fix.
1 — a venue that has never succeeded has no series at all
markIngestSuccess is only reached from writeVenuePricePoints, after a successful write. Nothing initialises the gauge, so until a venue's first successful cycle, ingest_last_success_timestamp{venue="soroswap",network="mainnet"} does not exist.
That breaks precisely the case the issue exists for. The alert anyone writes is:
time() - ingest_last_success_timestamp > 600
An expression over a series that does not exist returns no rows, and a rule with no rows does not fire. So a venue broken since startup — misconfigured endpoint, bad credentials, a crash loop on its first cycle — is invisible, while a venue that worked once and then died is caught. The silent case is the one you would most want to hear about.
Initialise the gauge for every (venue, network) pair the runner starts, at startup. 0 or the process start time both work; say which you chose and why in a comment, because the alert expression depends on it.
Worth checking ingest_errors_total for the same shape — a counter that never increments is a missing series too, though it matters less since alerts on it are usually rate() > 0.
2 — no tests
There are none in the diff. The issue asked for one specifically:
- A test asserts lag increases when no write occurs
That is the assertion that would have caught point 1, which is a fair demonstration of why it was on the list.
Please cover: the timestamp advances after a successful write, the series exists before any write has happened, a cycle records a duration, and a thrown error increments the counter under the right category. The category classifier in particular is regex-based —
const category = /429|rate.?limit/i.test(error.message) ? 'rate_limit' :
/timeout|timed out|abort/i.test(error.message) ? 'timeout' : 'other'— and a test naming the strings it expects to classify is what stops a library upgrade silently reclassifying every rate limit as other.
One note while you are in there: an AbortSignal.timeout rejection throws a TimeoutError whose message is "The operation was aborted due to timeout". abort matches, so that works today — but it works by luck rather than by design. Checking error.name === 'TimeoutError' first, before falling back to the text, would make it deliberate. #379 landed those timeouts earlier today, so this path is live now.
Note on #397
Your other PR touches src/ingest/venues/amm.ts and runner.ts as well. Whichever of these two lands first, the other will need a rebase — worth knowing before you push the fix.
e97fef7 to
fa344f7
Compare
|
@Miracle656 The branch has been rebased onto the latest main and the conflicts are resolved. Please review and merge when ready. |
Adds bounded-cardinality metrics for successful venue writes, ingestion cycle duration, and categorized cycle failures. No issue-closing keyword is included.
Closes #340