feat: add http rsr #285
feat: add http rsr #285
Conversation
@NikolasHaimerl what does MR stand for? |
Just a habit from from working with Gitlab: MR= Merge Request, PR = Pull request. Means the same thing |
Co-authored-by: Julian Gruber <[email protected]>
stats/lib/stats-fetchers.js
Outdated
| success_rate: r.total > 0 ? r.successful / r.total : null, | ||
| successful_http: r.successful_http ?? null, | ||
| // successful_http might be null because the column was added later | ||
| success_rate_http: r.total > 0 && !(r.successful_http === undefined || r.successful_http === null) ? r.successful_http / r.total : null |
There was a problem hiding this comment.
| success_rate_http: r.total > 0 && !(r.successful_http === undefined || r.successful_http === null) ? r.successful_http / r.total : null | |
| success_rate_http: r.total > 0 && typeof r.successful_http === 'number' ? r.successful_http / r.total : null |
I think this is simpler and still covers all cases
There was a problem hiding this comment.
Not sure if this works. successful_http is a string representation of a number.
We could use something like this for the check to be more concise and reuse the expression:
const isValidNumber = value => value != null && !isNaN(+value)
And then call:
success_rate_http: r.total > 0 && isValidNumber(r.successful_http) ? r.successful_http / r.total : null
There was a problem hiding this comment.
FYI:
node-pg represents 64-bit integers coming from Postgres as JavaScript strings to preserve precision.
Even if the column successful_http is represented as a 32-bit integer, SUM(successful_http) yields a 64-bit integer.
We could cast successful_http to INT in the Postgres query so that we receive a Number on the JavaScript side, but that would create an inconsistency with how we handle successful, so this is not a good solution.
I think we can trust that the database returns either null (when the column value is not set) or a string representation of a number.
| success_rate_http: r.total > 0 && !(r.successful_http === undefined || r.successful_http === null) ? r.successful_http / r.total : null | |
| success_rate_http: r.total > 0 && r.successful_http !== null ? r.successful_http / r.total : null |
Co-authored-by: Miroslav Bajtoš <[email protected]>
…tation/spark-stats into nhaimerl-add-http-rsr
| { day, success_rate: 0.1, successful: '1', total: '10', successful_http: '1', success_rate_http: 0.1 } | ||
| ]) | ||
| }) | ||
| it('handles successful_http values 0, null, undefined', async () => { |
There was a problem hiding this comment.
Please add a similar test for per-miner RSR endpoint. I find it important to verify that SUM() and our logic detecting null values work as expected.
stats/test/handler.test.js
Outdated
|
|
||
| res = await fetch( | ||
| new URL( | ||
| '/miners/retrieval-success-rate/summary?from=2024-01-20&to=2024-01-22', |
There was a problem hiding this comment.
This test case (it('handles successful_http values 0, null, undefined',) is nested inside the test suite describe('GET /retrieval-success-rate',. It should test the endpoint GET /retrieval-success-rate only.
Please move this new block testing /miners/retrieval-success-rate/summary to a new test case inside the test suite describe('GET /miners/retrieval-success-rate/summary' later in this file.
spark-stats/stats/test/handler.test.js
Line 463 in f4918ee
This MR proposes the following changes:
Related to Issue #192