Skip to content
This repository has been archived by the owner on Jun 18, 2024. It is now read-only.

Fix v8 tableresult empty #239

Merged
merged 8 commits into from
Apr 7, 2022
Merged

Fix v8 tableresult empty #239

merged 8 commits into from
Apr 7, 2022

Conversation

peishaofeng
Copy link
Contributor

Overview

  • auto-convert DataFrame query format to TimeSeries on Grafana v8.*
  • fix prettier/eslint errors
  • upgrade @grafana SDK to ~8.2.5

What this PR does / why we need it

To fix issue #227 Empty results in status map panel post grafana version upgrade from v7.4 to v8.1. #227

Special notes for your reviewer

1. auto-convert data format according Grafana version

on v7 keep existing data format, no conversion.

on v8 made changes below:

  • listen on new event PanelEvents.dataFramesReceived for DataFrame format
  • convert DataFrame to TimeSeries format with DataProcessor
  • copy data_processor.ts from Grafana graph panel plugin with minor modification

${GRAFANA}/public/app/plugins/panel/graph/data_processor.ts

I have verified the same plugin package build on both Grafana v8.0.6 and v7.4.2, it works fine.

2. SDK 8.3.0 report fail

@grafana SDK 8.3.0 report error, so i use 8.2.5 instead

✖ PostCSS plugin postcss-discard-comments requires PostCSS 8.
Migration guide for end-users:
https://github.com/postcss/postcss/wiki/PostCSS-8-for-end-users
  Trace: Error: PostCSS plugin postcss-discard-comments requires PostCSS 8.
  Migration guide for end-users:
  https://github.com/postcss/postcss/wiki/PostCSS-8-for-end-users

Does this PR introduce a user-facing change?

NONE

@diafour
Copy link
Collaborator

diafour commented Dec 6, 2021

Thank you for this PR! DataFrames support is a long-awaited feature.

I've got some findings about data frames and Grafana versions (not precise, sorry):

  1. Panels plugins with dataFrames are supported since Grafana 6.7.
  2. @grafana/data module appears in plugins with migration to PanelEvents.
  3. Adding import @grafana/data make plugin not working in Grafana <=6.3. It is not possible to check if module is not injected and change plugin behavior for that older versions.

How these facts affect your PR?

  1. Update README.md: set minimum supported version to Grafana 6.7.
  2. V8 detection is not needed, use data frames for data handling. Set this.useDataFrames = true and remove onDataReceived.
  3. Import PanelEvents and CoreEvents from @grafana/data, no need to fallback to string-based events. Remove all Polygrafill hackery: fallbackToStringEvents, ./util/grafana/events, etc.

Do you have time to make these changes?

@peishaofeng
Copy link
Contributor Author

Yes, I'd like to be of help to make this panel plugin better.
Also need to prepare both v6 & v7 environment to verify modification, next week should have spare time to do further optimization as you mentioned above.

peishaofeng added 2 commits December 10, 2021 16:37
- replace all self-defined system events with @grafana/data PanelEvents
- remove all prior v6.7 event polyfill
- prefix graphHover & graphHoverClear event name with *statusmap*
- update README.md & plugin.json, require grafana v6.7+
@peishaofeng
Copy link
Contributor Author

submit changes according your suggestion, the same plugin build are verified on v6.7.1/v7.4.2/v8.2.5

1. convert DataFrame to series list

  • useDataFrames for version > v7.0
  • add DataProcessor to convert query result in type of DataFrame to series list(TimeSeries2)
  • register new data events handler dataFramesReceived & dataSnapshotLoad for DataFrame type
  • remove prefixed column name in series tag name in v8.0+, keep compatible with v7.x series name

2. remove string-based events polyfill

  • replace all self-defined PanelEvents (refresh/render/dataReceived etc) with @grafana/data PanelEvents
  • remove fallbackToStringEvents
  • prefix self-defined event names(graphHover and graphHoverClear) with statusmap

Grafana v6.* has built-in graphHover and graphHoverClear defined which cause event name conflict with self-defined events.

3. move renderComplete to utils/grafana/events/events.ts

  • collect all self-defined events in utils/grafana/events/events.ts

4. fix prettier warnings

  • fix arrowParens warning

5. README.md

  • Supported Grafana versions:
    • 6.7+ to 8.2+
    • prior 6.7 will cause plugin load fail (no support or incompatible @grafana/data and @grafana/runtime package)

6. plugin.json

  "dependencies": {
    "grafanaDependency": ">=6.7.0",
    "grafanaVersion": "6.7+",
    "plugins": [ ]
  }

Copy link
Collaborator

@diafour diafour left a comment

Choose a reason for hiding this comment

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

OK, my eyes see no critical issues. I change README to something more user-friendly. I want to run some tests on different versions before merging.

Comment on lines +219 to +224
console.log('Grafana buildInfo:', config.buildInfo);

const majorVersion = parseInt(config.buildInfo.version.split('.', 2)[0], 10);
if (majorVersion <= 6) {
// data will fall into onDataReceived() with series data format
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Grafana 6.7 doesn't work if plugin sets this.useDataFrames = true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on v6.7 although MetricsPanelCtrl has member useDataFrames defined, but PanelEvents.dataFramesReceived still not emerge, register this event will raise exception, neither onDataFramesReceived nor onDataReceived can receive query result, I guess DataFrame is formally released from v7.0?

statusmap-onDataReceived-v671-useDataFrames-true-render-log

@diafour diafour added this to the 0.5.0 milestone Dec 13, 2021
@diafour diafour merged commit a7782f3 into flant:master Apr 7, 2022
@peishaofeng peishaofeng deleted the fix-v8-tableresult-empty branch May 10, 2022 09:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants