(wip): misc(frontend): Add a useTimeseries hook #89417
Conversation
github-actions
bot
added
the
Scope: Frontend
| /** | ||
| * Row represents a point in a timeseries - plotting (timestamp, value) with a line between them would represent a line chart. | ||
| */ |
There was a problem hiding this comment.
I don't think this is even needed
| /** | ||
| * Row represents a point in a timeseries - plotting (timestamp, value) with a line between them would represent a line chart. | ||
| */ | ||
| export interface Row { |
There was a problem hiding this comment.
This is more of a general note, but could you align this a little more with the existing TimeSeries types? e.g., this should be called TimeSeriesItem, SeriesMeta should be called TimeSeriesMeta, etc.
| timestamp: number; | ||
|
|
||
| /** value: The float value of this point in the time series (i.e., how high it is in the graph) */ | ||
| value: number; |
There was a problem hiding this comment.
| value: number; | |
| value: number | null; |
| */ | ||
| export interface SeriesMeta { | ||
| /** valueType: The type of the unit, e.g., 'duration' or 'size' */ | ||
| valueType: string; |
There was a problem hiding this comment.
This should be a string union of the known types, you can copy AttributeValueType from TimeSeries, or import it
| order?: number; | ||
|
|
||
| /** valueUnit: The unit of this chart, e.g., 'microsecond' */ | ||
| valueUnit?: string; |
There was a problem hiding this comment.
This should be a union of the known units, you can copy TimeSeriesValueUnit from TimeSeries or import it
| field?: string[]; | ||
|
|
||
| /** The keys to order by, e.g., -count() */ | ||
| orderby?: string[]; |
There was a problem hiding this comment.
😭 is it too late to make this orderBy?
| }, | ||
| }, | ||
| ], | ||
| {} |
There was a problem hiding this comment.
This needs a little more configuration.
- Retries for 409s, with a falloff (
doDiscoverQuerydoes this) - Intelligent
staleTime(useSpansQuerydoes this)
| timeseries: TimeSeries[]; | ||
| } | ||
|
|
||
| export interface UseTimeseriesProps { |
There was a problem hiding this comment.
This needs a mandatory referrer parameter, please!
| yAxis?: string[]; | ||
| } | ||
|
|
||
| export function useTimeseries(props: UseTimeseriesProps) { |
There was a problem hiding this comment.
Maybe useDiscoverTimeSeries or useEAPTimeSeries? useTimeseries is not specific enough
| const {selection} = usePageFilters(); | ||
| const {start: pageFilterStart, end: pageFilterEnd} = selection.datetime; | ||
| const organization = useOrganization(); | ||
| return useApiQuery<StatsResponse>( |
There was a problem hiding this comment.
Please do a small transformation here, and convert the timeseries in the response into TimeSeries as defined in the widget code. You'll have to do that anyway to plot it, and I don't want yet another data format getting passed around.
It'll be easy, you'll need to concatenate the groupBy and yaxis into a field, convert the timestamps into strings and ... I think that's it actually?
I'll slowly chip away at aligning this time series with TimeSeries in the near future
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.