Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Table: Highlight row on shared crosshair #78392

Merged
merged 21 commits into from Dec 13, 2023

Conversation

mdvictor
Copy link
Contributor

@mdvictor mdvictor commented Nov 20, 2023

What is this feature?

Adds shared crosshair support to the table panel. If the table has a time field, it will be able to highlight rows based on it and will also set the crosshair on the equivalent datapoint in other panels (e.g.: timeseries) on row hover.

Screen.Recording.2023-11-16.at.13.07.13.mov

Why do we need this feature?

Enhance table panel functionality.

Who is this feature for?

Everyone

Which issue(s) does this PR fix?:

Fixes #7310

Special notes for your reviewer:

TODO:

  • Add feature toggle

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@mdvictor mdvictor added this to the 10.2.x milestone Nov 20, 2023
@mdvictor mdvictor self-assigned this Nov 20, 2023
@mdvictor mdvictor requested review from a team, codeincarnate, oscarkilhed and baldm0mma and removed request for a team November 20, 2023 13:21
@grafana-delivery-bot grafana-delivery-bot bot modified the milestones: 10.2.x, 10.3.x Nov 20, 2023
@mdvictor
Copy link
Contributor Author

The DataHoverEvent also seems to send a rowIndex param which can be useful when the same dataFrame is used for a table panel and, let's say a timeseries panel. Or when two dataframes are similar (e.g: have the same time field values and order). If the time field order doesn't match in the table, we fall back to searching the time value of the hovered point in a timeseries chart in the table rows and highlight a row if we find a match.

Copy link
Contributor

@oscarkilhed oscarkilhed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Take a look at the debounceTime comment, after addressing that I think this is good to go. 🎉

subs.add(
panelContext.eventBus
.getStream(DataHoverEvent)
.pipe(throttleTime(50))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest replacing throttleTime with debounceTime and increasing the timeout to something like 250-500 ms.

throttleTime will take one event and ignore the rest for 50 milliseconds. debounceTime will ignore events until there hasn't been a new event in 50 milliseconds.

throttleTime has the effect that if a user drags his/her mouse over a bunch of points and then stops, it might not be the point the mouse is over when the users stops dragging.

debounceTime has the effect that if the user drags over a bunch of points they will be ignored until the mouse is stopped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! Will look into that, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested out the debounceTime function but it adds a weird delay into the mix. My understanding of this func is that it only emits a value after a given time period passes without any new emits. So hovering over a point in a graph will result in a row highlight only after the timeout passes with no new events(e.g.: after half a second for debounceTime(500)).

I would leave the throttle as it is to be in sync with how GraphNG deals with DataHoverEvent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the delay is something we would want here, the constant redrawing of the table is quite resource intense. Maybe lower the debounceTime from 500 to 100-200? Even if we just set it to 50 ms, debounceTime is going to highlight the correct point every time the mouse stops. With throttleTime, when the mouse stops on a point in the graph, the highlighted row is going to correspond to what was hovered 50ms ago, not what the mouse is currently hovering.

throttleTime might look better, but it's producing wrong results

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, i agree. Switched to a debounceTime of 100.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@torkelo wants to switch the default to setting of dashboards to shared cursor, so performance here will be important.

#78636

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we already throttle these eventbus events during emission. i think adding a second level of thottling wont be too good. maybe we can just increase the throttle time during emission?

Comment on lines +550 to +569
export function calculateAroundPointThreshold(timeField: Field): number {
let max = -Number.MAX_VALUE;
let min = Number.MAX_VALUE;

if (timeField.values.length < 2) {
return 0;
}

for (let i = 0; i < timeField.values.length; i++) {
const value = timeField.values[i];
if (value > max) {
max = value;
}
if (value < min) {
min = value;
}
}

return (max - min) / timeField.values.length;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works a lot better! 🥇

@mdvictor
Copy link
Contributor Author

mdvictor commented Nov 27, 2023

i guess i wasn't remembering this correctly, we don't throttle the publish, just the subscribe 😞

https://github.com/grafana/grafana/blob/main/public/app/core/components/GraphNG/GraphNG.tsx

Any hints/tips?

this might be a tough one to fix with React's rendering model if it has to re-render the whole table. i think there might be some big wins from extracting RenderRow() into its own component that's carefully wrapped with React.memo. cc @gtk-grafana

https://github.com/grafana/grafana/blob/main/packages/grafana-ui/src/components/Table/Table.tsx#L241-L290

@leeoniya I think memoing rows won't work because of the virtualised list. It only renders what is in the table view, so the index will keep updating as you scroll, or as the dataHoverEvent picks a new hover row and scrolls there. I've broken the row list part in a new component which rerenders on row hover but leaves the rest of the table alone, same as with a simple table scroll, so I think this should work performance-wise. There's still more stuff that could be moved from the parent component in here (e.g.: the actual RenderRow method), to better break the component. Overall, I think this could be a way forward.

onRowHover={config.featureToggles.tableSharedCrosshair ? onRowHover : undefined}
onRowLeave={config.featureToggles.tableSharedCrosshair ? onRowLeave : undefined}
enableSharedCrosshair={config.featureToggles.tableSharedCrosshair && enableSharedCrosshair}
eventBus={panelContext.eventBus}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that this can be also used in other places, potentially? Otherwise we could drop this prop and call the one in usePanelContext() within RowsList

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think ever should think of that , if visualizations are used outside a panel they need to be wrapped in PanelContext at least to access those features that use it

onRowHover?: (idx: number, frame: DataFrame) => void;
onRowLeave?: () => void;
/** Used to highlight a row at a given index */
rowHighlightIndex?: number;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be used standalone to highlight a row in some other scenarios. If set it overwrites the one received through the data hover event. Not sure if could be useful or if it's too much.

@leeoniya
Copy link
Contributor

leeoniya commented Nov 28, 2023

seeing some weird data flashing/changing in this test dashboard. is it trying to scroll the virtualized table to the row? but then stops working once it gets far enough in time?

table-shared-crosshair-perf.json
{
  "annotations": {
    "list": [
      {
        "builtIn": 1,
        "datasource": {
          "type": "grafana",
          "uid": "-- Grafana --"
        },
        "enable": true,
        "hide": true,
        "iconColor": "rgba(0, 211, 255, 1)",
        "name": "Annotations & Alerts",
        "type": "dashboard"
      }
    ]
  },
  "editable": true,
  "fiscalYearStartMonth": 0,
  "graphTooltip": 1,
  "id": 850,
  "links": [],
  "liveNow": false,
  "panels": [
    {
      "datasource": {
        "type": "grafana-testdata-datasource",
        "uid": "PD8C576611E62080A"
      },
      "fieldConfig": {
        "defaults": {
          "color": {
            "mode": "thresholds"
          },
          "custom": {
            "align": "auto",
            "cellOptions": {
              "type": "auto"
            },
            "inspect": false
          },
          "mappings": [],
          "thresholds": {
            "mode": "absolute",
            "steps": [
              {
                "color": "green",
                "value": null
              },
              {
                "color": "red",
                "value": 80
              }
            ]
          }
        },
        "overrides": []
      },
      "gridPos": {
        "h": 15,
        "w": 22,
        "x": 0,
        "y": 0
      },
      "id": 2,
      "options": {
        "cellHeight": "sm",
        "footer": {
          "countRows": false,
          "fields": "",
          "reducer": [
            "sum"
          ],
          "show": false
        },
        "showHeader": true
      },
      "pluginVersion": "10.3.0-pre",
      "targets": [
        {
          "datasource": {
            "type": "grafana-testdata-datasource",
            "uid": "PD8C576611E62080A"
          },
          "refId": "A",
          "scenarioId": "random_walk",
          "seriesCount": 7
        }
      ],
      "title": "Panel Title",
      "transformations": [
        {
          "id": "joinByField",
          "options": {}
        }
      ],
      "type": "table"
    },
    {
      "datasource": {
        "type": "grafana-testdata-datasource",
        "uid": "PD8C576611E62080A"
      },
      "fieldConfig": {
        "defaults": {
          "color": {
            "mode": "palette-classic"
          },
          "custom": {
            "axisBorderShow": false,
            "axisCenteredZero": false,
            "axisColorMode": "text",
            "axisLabel": "",
            "axisPlacement": "auto",
            "barAlignment": 0,
            "drawStyle": "line",
            "fillOpacity": 0,
            "gradientMode": "none",
            "hideFrom": {
              "legend": false,
              "tooltip": false,
              "viz": false
            },
            "insertNulls": false,
            "lineInterpolation": "linear",
            "lineWidth": 1,
            "pointSize": 5,
            "scaleDistribution": {
              "type": "linear"
            },
            "showPoints": "auto",
            "spanNulls": false,
            "stacking": {
              "group": "A",
              "mode": "none"
            },
            "thresholdsStyle": {
              "mode": "off"
            }
          },
          "mappings": [],
          "thresholds": {
            "mode": "absolute",
            "steps": [
              {
                "color": "green",
                "value": null
              },
              {
                "color": "red",
                "value": 80
              }
            ]
          }
        },
        "overrides": []
      },
      "gridPos": {
        "h": 17,
        "w": 22,
        "x": 0,
        "y": 15
      },
      "id": 1,
      "options": {
        "legend": {
          "calcs": [],
          "displayMode": "list",
          "placement": "bottom",
          "showLegend": true
        },
        "tooltip": {
          "mode": "single",
          "sort": "none"
        }
      },
      "targets": [
        {
          "datasource": {
            "type": "grafana-testdata-datasource",
            "uid": "PD8C576611E62080A"
          },
          "refId": "A",
          "scenarioId": "random_walk",
          "seriesCount": 1
        }
      ],
      "title": "Panel Title",
      "type": "timeseries"
    }
  ],
  "refresh": "",
  "schemaVersion": 39,
  "tags": [],
  "templating": {
    "list": []
  },
  "time": {
    "from": "now-6h",
    "to": "now"
  },
  "timepicker": {},
  "timezone": "",
  "title": "table-shared-crosshair-perf",
  "uid": "fa36a3f9-9b0f-49b7-97cf-f5f721bb5867",
  "version": 3,
  "weekStart": ""
}
Peek.2023-11-28.13-44.mp4

subs.add(
panelContext.eventBus
.getStream(DataHoverClearEvent)
.pipe(throttleTime(50))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this should also be debounced

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this per Leon's comment, but maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this should be debounced! Thank you for pointing it out, will work on it! 🎉

@@ -533,3 +533,37 @@ export function getAlignmentFactor(
return alignmentFactor;
}
}

export function hasTimeField(data: DataFrame): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should probably be in @grafana/data as we have functions like this there already


const timeField: Field = frame!.fields.find((f) => f.type === FieldType.time)!;

panelContext.eventBus.publish(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit complex to have logic split between internal Table component and the outer TablePanel.

Can't it all be on handled inside the table component, any benefit to having the subscribe there and the publish here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, there's no point in having this here. I must've overlooked this bit of code after the table breakdown. Will move it accordingly.

@mdvictor mdvictor requested review from a team as code owners November 29, 2023 11:21
@mdvictor mdvictor requested review from Clarity-89, L-M-K-B and mckn and removed request for a team November 29, 2023 11:21
@mdvictor
Copy link
Contributor Author

seeing some weird data flashing/changing in this test dashboard. is it trying to scroll the virtualized table to the row? but then stops working once it gets far enough in time?

table-shared-crosshair-perf.json
Peek.2023-11-28.13-44.mp4

@leeoniya are you referring to the weird hover at the row in the middle of the view? It should scroll to the hovered row at all times, shouldn't stop when it gets far enough in time. Need to look more into that, might need to raise the debounce potentially.

Copy link
Contributor

@leeoniya leeoniya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mdvictor mdvictor merged commit 5aff338 into main Dec 13, 2023
19 checks passed
@mdvictor mdvictor deleted the mdvictor/table-ts-shared-crosshair branch December 13, 2023 09:33
@summerwollin summerwollin modified the milestones: 10.3.x, 10.3.0 Jan 22, 2024
@leeoniya leeoniya mentioned this pull request Feb 28, 2024
51 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] Highlight row in table/log panel based on shared crosshair
7 participants