Alternatives for serving plugin

[marefr]

Suggestion to make it easier for a plugin developer to understand what interfaces they need to implement for a certain plugin type (datasource/app) and what interfaces we want to enforce a certain plugin to implement I started to think about using a "base struct" for default implementations. Struggled a lot with this "base struct" without being satisfied until I came to the conclusion that in general composition in favorable over inheritance.

In addition #96 suggested adding a new CheckDataSourceHealth to the existing QueryDataHandler to try and overcome the "weirdness" of having both a PluginConfig and a DataSourceConfig.

I think having a single interface that must be implemented per plugin type is desirable.

Current way of serving a plugin

https://github.com/grafana/google-sheets-datasource/blob/1e50d97f3bfd40870997039b17c0c8c901f8fbf8/pkg/main.go#L11-L27

Weakness with current approach is that there's nothing enforcing you to implement one interface, instead there's 4 different interfaces in backend.ServeOpts and it's probably not clear from a plugin developer perspective which ones I need/should implement.

Example 1

Must implement backend.DataSourcePlugin interface to be able to serve data source plugin.

package myplugin

import (
  "github.com/grafana/grafana-plugin-sdk-go/backend"
  "github.com/grafana/grafana-plugin-sdk-go/resource"
  "github.com/grafana/grafana-plugin-sdk-go/diagnostics"
)

type MyDataSource struct {
  CheckHealthHandler
  CallResourceHandler 
  logger log.Logger
}

func New(logger log.Logger, c backend.ConfigurePlugin) backend.DataSourcePlugin {
  c.MustRegister(<prometheus metrics>)

  // only implements QueryDataHandler by using composition and add default handlers for check health and call resource.
  return &MyDataSource{
    CheckHealthHandler: diagnostics.OKCheckHealthHandler(),
    CallResourceHandler: resource.NotFoundHandler(),
    logger: logger,
  }
}

func (ds *MyDataSource) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {
  ...
}

package main

import (
  "github.com/grafana/grafana-plugin-sdk-go/backend"
)

func main() {
  backend.ServeDataSourcePlugin(myplugin.New)
}

Example 2

Using new datasource package with explicit datasource.Plugin interface. Must implement datasource.Plugin interface to be able to serve data source plugin.

package myplugin

import (
  "github.com/grafana/grafana-plugin-sdk-go/backend"
  "github.com/grafana/grafana-plugin-sdk-go/resource"
  "github.com/grafana/grafana-plugin-sdk-go/diagnostics"
)

type MyDataSource struct {
  datasource.CheckDataSourceHealthHandler
  datasource.CallDataSourceResourceHandler 
  logger log.Logger
}

func New(logger log.Logger, c backend.ConfigurePlugin) datasource.Plugin {
  c.MustRegister(<prometheus metrics>)

  // only implements QueryDataHandler by using composition and add default handlers for check health and call resource.
  return &MyDataSource{
    CheckDataSourceHealthHandler: datasource.NewCheckDataSourceHealthHandlerFunc(diagnostics.OKCheckHealthHandler()),
    CallDataSourceResourceHandler: datasource.NewCallDataSourceResourceHandlerFunc(resource.NotFoundHandler()),
    logger: logger,
  }
}

func (ds *MyDataSource) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {
  ...
}

package main

import (
  "github.com/grafana/grafana-plugin-sdk-go/datasource"
)

func main() {
  datasource.Serve(myplugin.New)
}

[bergquist]

Great work! I like this. Make it ready for review? :)

[kylebrandt]

Comments please for these exported interfaces and the types that Follow

[marefr]

Yeah good point, but still a draft. So only consider the concept/examples

[kylebrandt]

// Package datasource ....

[marefr]

The idea would be to stick with example 1 or example 2, not both. Same changes would be needed for both app and transform. Or keep it as is.

[marefr]

Only OrgID and DataSourceConfig exposed here compared to regular CallResourceRequest which includes both PluginConfig and DataSourceConfig.

Helps not confusing plugin developer to use PluginConfig (currently only app plugins) when should be using DataSourceConfig.

[marefr]

Not super found of this mapping back and forth, but removes the need of implementing yet another interface in resource and httpadapter packages.

[marefr]

To be correct, this one should probably have a new interface in this package as well to only include DataSourceConfig in request and not PluginConfig.

[marefr]

Could be an interface similar to https://github.com/grafana/grafana/blob/master/pkg/tsdb/query_endpoint.go#L11, basically using similar DataSource model struct.

[kylebrandt]

Trying to think through using interfaces, and non-breaking upgrades. For example with the datasource.Plugin interface.

1. We release 1.0 with this interface
2. We release 1.1, adding a new RPC method, which means a new handler.
3. Plugin author fetches 1.1, which isn't supposed to have breaking changes

However, at step 3, wouldn't the plugin author get something like "MyDataSource is not a datasource.Plugin, missing QuerySloth(...) ...`

Edit: Looking at "Interfaces" section of https://about.sourcegraph.com/go/gophercon-2019-detecting-incompatible-api-changes

[ryantxu]

please please please please look at #105 and see if this is related.

Having an instance concept encapsulated by the API would be a really nice starting place

[bergquist]

@marefr lets close this?