StateTimeline: Treat second time field as state endings #84130
Conversation
leeoniya
added
type/bug
area/panel/state-timeline
no-backport
leeoniya
requested review from
Develer,
mdvictor and
kaydelaney
and removed request for
a team
leeoniya
requested review from
nmarrs
and removed request for
mdvictor and
kaydelaney
| @@ -496,6 +545,7 @@ export function prepareTimelineFields( | |||
| }, | |||
| }, | |||
| }; | |||
| changed = true; | |||
There was a problem hiding this comment.
this is the fix for #76869
There was a problem hiding this comment.
Tested locally and it looks like it addresses the escalation - it might be a good idea to sync up as a team around null handling as its been several months since we poked at this and it would be good for everyone to fully understand the 3 different null modes
| } else if (endFieldIdx === -1) { | ||
| endFieldIdx = i; | ||
| break; | ||
| } |
There was a problem hiding this comment.
I remember that we have handling were we have consider the first field to be the time field, does it make sense though to always consider the next found time field as the end field? (i.e. couldn't other time data be represented in user's data?)
I'm not sure this should be default behavior vs only apply in certain circumstances 🤷🏻♂️
There was a problem hiding this comment.
i.e. couldn't other time data be represented in user's data?
technically, yes. but there is plenty of existing "auto" behavior that makes assumptions like "first string field", "all number fields", etc. to get it perfect in all cases we'd have to give explicit control to users over what each field in their response represents for each viz panel.
my gut feeling is that endTime is the most likely additional time field. if this turns out to be wrong, we can then expose options to override these assumptions and select fields explicitly, like we do in Trend, Candlestick, XYChart, etc.
| ['OK', undefined, null, undefined, 'NO_DATA', undefined, undefined, null], | ||
| [undefined, 'ERROR', undefined, null, undefined, 'WARNING', null, undefined], | ||
| ]); | ||
| }); |
yep. but i'm also secretly hoping to ride it out till uPlot v2 and we don't need field joining / frame alignment any more. then there is only |
Fixes https://github.com/grafana/support-escalations/issues/9636
Fixes #84127
StateTimeline panel currently requires either
nullvalues or a new state to terminate the previous state. however, splicing in the null values into the correct positions is pretty much impossible with our transformations. datasets often contain some form ofendTimefield to indicate state terminations. what this PR does is detects the presence of a second time field and splices in the null values correctly (using our belovedouterJointransformation).additionally, it fixes a regression i introduced in #83355 that removed
spanNulls: -1handling from GraphNG. it turns out we still need this while we do the final frame joining in lower layers and this is the only way that StateTimeline prep is able to communicate theNULL_RETAINintent. we can remove this once we pull up the frame alignment up into the panels. i added a test to make sure we test the intended behavior and not thespanNullssetting itself, which is an implementation detail.new gdev dashboard that shows off the new endTime detection and requires that frame joining +
NULL_RETAINworks properly.also fixes another
spanNulls-related bug...Fixes #76869