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

Explore: Allow the use of plugin panels #66982

Merged
merged 7 commits into from
Jul 7, 2023
Merged

Explore: Allow the use of plugin panels #66982

merged 7 commits into from
Jul 7, 2023

Conversation

Umaaz
Copy link
Contributor

@Umaaz Umaaz commented Apr 20, 2023

Allow plugins to define a visualisation to use in explore that comes from a plugin.

What is this feature?

This PR allows a datasource plugin to specify a panel from a panel plugin to use when rendering the query result on the explore page.

By setting frame.meta.preferredVisualisationType to plugin and adding frame.meta.custom.pluginID to the dataframe response from the datasource. The explore view can load the appropriate panel.

Why do we need this feature?

I needed this feature, as I am developing a datasource that stores data that doesn't fall into the categories provided by the current explore page.

Who is this feature for?

Users that are developing datasources that provided data that requires specific rendering, and want to take advantage of the explore functionality.

Which issue(s) does this PR fix?:

I could not find any specific issues that relate to this, possibly relates to #58849, but not sure.

Fixes #

Special notes for your reviewer:

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.

Allow plugins to define a visualisation to use in explore that comes from a plugin.
@Umaaz Umaaz requested review from a team as code owners April 20, 2023 15:22
@Umaaz Umaaz requested review from joshhunt, JoaoSilvaGrafana and andresmgot and removed request for a team April 20, 2023 15:22
@CLAassistant
Copy link

CLAassistant commented Apr 20, 2023

CLA assistant check
All committers have signed the CLA.

@grafanabot grafanabot added area/explore area/frontend pr/external This PR is from external contributor labels Apr 20, 2023
@academo
Copy link
Member

academo commented Apr 21, 2023

@Umaaz can you provide a code example of how this looks from the plugin's code perspective?

@@ -58,6 +59,11 @@ export const decorateWithFrameTypeMetadata = (data: PanelData): ExplorePanelData
case 'flamegraph':
flameGraphFrames.push(frame);
break;
case 'plugin':
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this potentially could be part of the default case and if set, try using the plugin id and load the plugin. Then preferredVisualization type could support any plugin id, rather than introducing a type=plugin in the mix? Guess it might complicate things?

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 did do that originally, then thought it would be better to have a explicit type=plugin to prevent any chance of existing plugins causing issues. In the very small chance that some plugin exists with meta.custom.pluginID.

@Umaaz
Copy link
Contributor Author

Umaaz commented Apr 21, 2023

@Umaaz can you provide a code example of how this looks from the plugin's code perspective?

This is a snippet from the datasource plugin I am developing. The plugin 'intergralgmbh-grafanadeep-panel' is another plugin that provides the panel.

	frame := &data.Frame{
		Name: "Snapshot",
		Fields: []*data.Field{
			data.NewField("ID", nil, []string{}),
                        ...
		},
		Meta: &data.FrameMeta{
			PreferredVisualization: "plugin",
			Custom: map[string]interface{}{
				"pluginID": "intergralgmbh-grafanadeep-panel",
				"title":    fmt.Sprintf("Snapshot: %v:%v", snap.Tracepoint.Path, snap.Tracepoint.LineNumber),
			},
		},
	}

@gelicia
Copy link
Contributor

gelicia commented Apr 25, 2023

We need to discuss this a little more internally but I'm liking it.

Posting testing instructions if anyone else wants to play around.

Tested with the following:
TestData datasource, raw frames scenario
The following raw dataframe:

Dataframe
[
  {
    "meta": {
      "preferredVisualisationType": "plugin",
      "custom": {
        "pluginID": "piechart"
      }
    },
    "fields": [
      {
        "name": "time",
        "values": [
          1,
          2
        ]
      },
      {
        "name": "log",
        "values": [
          "app=foo level=error msg=\"Request\" endpoint=/test",
          "app=foo level=info msg=\"Deployed\""
        ]
      },
      {
        "name": "app",
        "values": [
          "foo",
          "foo"
        ]
      },
      {
        "name": "endpoint",
        "values": [
          "/test",
          null
        ]
      }
    ]
  }
]

Changed around the pluginID to 'table', 'logs', 'stat', 'cloudwatch', 'dashlist', etc
Explore did its best to render the data, throwing an error if it didn't work because the data wasn't formatted in a way the visualization would expect.

@torkelo
Copy link
Member

torkelo commented Apr 25, 2023

if we need a new custom meta field it might be good to add frame.meta.preferredVisualisationPluginId ?

<PanelChrome title={title} width={width} height={height} loadingState={state}>
{(innerWidth, innerHeight) => (
<PanelRenderer
data={{ series: [frame], state: state, timeRange }}
Copy link
Member

Choose a reason for hiding this comment

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

why is only a single frame passed to the visualization?

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 went this way, because each frame can have a different render. e.g the datasource could return 1 frame for plugin-1 and 1 frame for plugin-2.

@marefr
Copy link
Member

marefr commented Apr 25, 2023

Reminder. If merging this we need to update https://github.com/grafana/grafana-plugin-sdk-go/blob/0c55ef8790683bbf1f4aa26e7edb625becad9e2d/data/frame_meta.go#L54-L72

Given

if we need a new custom meta field it might be good to add frame.meta.preferredVisualisationPluginId ?

We need to add it to https://github.com/grafana/grafana-plugin-sdk-go/blob/0c55ef8790683bbf1f4aa26e7edb625becad9e2d/data/frame_meta.go#L14

Rename ExplorePanel to CustomContainer
@Umaaz
Copy link
Contributor Author

Umaaz commented May 29, 2023

Is there any update on if this can be accepted? Or if I need to make any changes?

@Elfo404
Copy link
Member

Elfo404 commented May 30, 2023

@Umaaz sorry for the delay, we are planning to take a better look at this soon, there are some unknowns we want to discuss with the team regarding Explore but a few people are off until next week.

@ifrost ifrost self-requested a review June 5, 2023 11:21
@ifrost
Copy link
Contributor

ifrost commented Jun 6, 2023

Thanks, @Umaaz for submitting it. It's a feature that was previously approached as well (e.g. #53600)

Why do we need this feature?
I needed this feature, as I am developing a datasource that stores data that doesn't fall into the categories provided by the current explore page.
Who is this feature for?
(...) want to take advantage of the explore functionality.

Just out of curiosity - what other Explore functionalities you will use in your use case? Have you considered writing an application plugin?

@Umaaz
Copy link
Contributor Author

Umaaz commented Jun 6, 2023

@ifrost essentially the use case of explore would be the same as say Tempo (traces). In this case there is a specific trace view that displays the trace in a way other than a table or chart.

I have the case that the snapshot data I want to render is difficult to render as a table or chart (as are traces). So I wanted to add a specific render (which I have via a panel plugin).

We have considered an App plugin, and will no doubt add one for some more specific functionality that would not make sense in Explore. We would still want the ability to use plugin panels in explore however, due to the reasons stated above.

If there are changes that are needed, or anything else I am able to do. Please let me know.

@ifrost
Copy link
Contributor

ifrost commented Jun 13, 2023

Thanks @Umaaz!

If there are changes that are needed, or anything else I am able to do. Please let me know.

This works really nice and I can see great value in it. I think there's a few things that should be addressed:

if we need a new custom meta field it might be good to add frame.meta.preferredVisualisationPluginId ?

I think this may work better as we can still use the preferredVisualizationType as the fallback. Users may use the data source plugin but not the visualization plugin, so we should have such fallback in place.

why is only a single frame passed to the visualization?

Explore groups data by preferredVisualizationType and the same should happen with preferredVisualisationPluginId. Each visualization plugin should be capable of handling multiple data frames. Essentially it means there will be one CustomContainer per preferredVisualisationPluginId.

"title": fmt.Sprintf("Snapshot: %v:%v", snap.Tracepoint.Path, snap.Tracepoint.LineNumber),

We don't have custom titles for visualizations in Explore. At the moment they are just generic titles indicating type of the data and would be good to keep it consistent. It'd be great to show the visualization plugin name as the title. If you need to show some extra context you can still pass it in meta and then consume it from the visualization plugin.

This is a change that would be available to use by other developers too, so I suggest marking it as @alpha (e.g. preferredVisualisationPluginId property). We also need some documentation on how to use it.

Changed CustomContainer to take all frames for plugin id.
Add field preferredVisualisationPluginId to frame metadata.
Updated decorators to check for plugin and fallback to preferredVisualisationType if plugin cannot be found.
Handle case where there are no custom frames
@Umaaz
Copy link
Contributor Author

Umaaz commented Jun 20, 2023

@ifrost I think I have addressed all of the points. Let me know if I missed anything. I have also created grafana/grafana-plugin-sdk-go#705 with the change for the go SDK.

I added docs for the new property (and added the alpha tag). Was there any other docs that needed updated?

Copy link
Contributor

@ifrost ifrost left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the feedback @Umaaz 🙌 I've left a few more small comments to clean in up a bit. Seems to work great!

@grafana/dashboards-squad / @grafana/grafana-frontend-platform: are you okay with changes in grafana-data?

public/app/features/explore/Explore.tsx Outdated Show resolved Hide resolved
public/app/features/explore/utils/decorators.ts Outdated Show resolved Hide resolved
@ifrost ifrost added this to the 10.1.x milestone Jul 4, 2023
@ifrost ifrost added add to changelog no-backport Skip backport of PR labels Jul 4, 2023
@ifrost
Copy link
Contributor

ifrost commented Jul 4, 2023

LGTM 👍 Please have a look at the conflicts (hopefully should be easy to resolve), I'll run the remaining PR checks and it should be good to go!

@Umaaz
Copy link
Contributor Author

Umaaz commented Jul 5, 2023

Awesome @ifrost. Thanks! I have (hopefully) resolved the conflicts.

Don't forget the Go sdk will need updated for this to work from Go plugins. grafana/grafana-plugin-sdk-go#705

@ifrost ifrost added breaking change Relevant for changelog generation and removed breaking change Relevant for changelog generation labels Jul 6, 2023
Copy link
Contributor

@ifrost ifrost left a comment

Choose a reason for hiding this comment

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

@ifrost ifrost merged commit 524f111 into grafana:main Jul 7, 2023
32 of 33 checks passed
polibb pushed a commit that referenced this pull request Jul 14, 2023
* Explore: Allow the use of plugin panels

Allow plugins to define a visualisation to use in explore that comes from a plugin.

* Explore: Allow the use of plugin panels

Rename ExplorePanel to CustomContainer

* Explore: Allow the use of plugin panels

Changed CustomContainer to take all frames for plugin id.
Add field preferredVisualisationPluginId to frame metadata.
Updated decorators to check for plugin and fallback to preferredVisualisationType if plugin cannot be found.

* Explore: Allow the use of plugin panels

Handle case where there are no custom frames

* Explore: Allow the use of plugin panels

Add test cases
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/explore area/frontend no-backport Skip backport of PR pr/external This PR is from external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants