feat: Allow metrics and ratios in NumberChart #314
Conversation
svc-shorpo
commented
Feb 18, 2024
•
svc-shorpo
commented
- make sure backwards compatible
- make sure previously created number charts can be edited
- make sure it works on chart explorer page
|
svc-shorpo
force-pushed
the
sr/hdx-506
branch
2 times, most recently
from
dfed288
to
04acfe2
Compare
packages/app/src/HDXNumberChart.tsx
Outdated
| ); | ||
| const isLogsChartApi = table === 'logs' && series.length === 1; | ||
|
|
||
| const { data, isError, isLoading } = isLogsChartApi |
There was a problem hiding this comment.
Do you think we can just use the multi-series for everything here?
There was a problem hiding this comment.
technically we can, but would it make sense performance-wise? locally multi-series api is slower
now that i think about it, would it make sense to pass startDate: endDate - granularity here to only get latest value?
There was a problem hiding this comment.
Oh yeah definitely it makes sense to only send one window's worth of data over.
One thing that might be weird is that the backend ts bucketing is based on fixed windows (ex. 0:05, 0:10, 0:15) whereas endDate - granularity might give you something like 0:07 - 0:12, which would incorrectly straddle two windows. I think there's a TS function to get the right granularity window, but alternatively you can just subtract 2x granularity to make sure we have at least 1 full granularity window.
The other weird thing would be the latest granularity window might have incomplete data... (ex. it's 0:12, and the last window is 0:10 to 0:12) I'm not sure what we can do about that per-se.
For multi-series, if we can, we should try to migrate as much possible into multi-series as possible, just from the maintenance perspective. Will ping you about the perf issues you're seeing though, as it shouldn't be noticeably slower.
There was a problem hiding this comment.
pushed changes. i think it works, but ran into this edge case:
if I use count or count_distinct aggfn, the data is different between useLogsChart and useMultiSeriesChart, since we don't pass granularity to useLogsChart. should we pass full startDate - endDate window and granularity: null for certain AggFn or what would be desired behavior be?
There was a problem hiding this comment.
Ahhh okay, now that I think about it more, I think it makes sense to actually continue to use the entire time range and set granularity to undefined. Does that break any of the metric charts?
There was a problem hiding this comment.
no, i think that works
There was a problem hiding this comment.
rebased and pushed changes (use entire time range and set granularity to undefined)
Going to remove `NumberChart` and `api.useMetricsChart` in #314
…perdxio#329) Going to remove `NumberChart` and `api.useMetricsChart` in hyperdxio#314
    - [x] make sure backwards compatible - [x] make sure previously created number charts can be edited - [x] make sure it works on chart explorer page
