Improve example – simplify code and add streaming functionality

[FZambia]

My initial goal was to add streaming example to a starter template. But while adding it I found several confusing problems:

• We suggest using instance manager but not showing how to do this
• We are using separate struct as management instance (instanceSettings) - it's a bit confusing how to get datasource setting inside SampleDatasource QueryData method (need to call instance manager with type assertions), also instanceSettings contains unused http client - also not really obvious why it's there
• I think that with current layout it's pretty non-obvious to do a proper resource cleanup when Dispose called (for example cleanup sth belonging to SampleDatasource instance) - thus we don't provide a straight way for correct cleanups.

I changed example a bit using a companion pr to SDK - example now uses automatic instance management hidden inside new datasource.Manage method. I believe this simplifies plugin code drastically.

The problem with this approach I see is that real plugin capabilities are not known (since we don't have access to datasource instance till first request from Grafana, automatic instance manager implements all capabilities). If this is a real problem then maybe we can add an option where developer explicitly tells which capabilities datasource instance provides. The current approach solves this but in a way which is pretty confusing.

[ryantxu]

nice

[ryantxu]

does this include the magic GRPC sauce? I think it should

[FZambia]

not yet, but experimental.ManageGRPC added in SDK

[marcusolsson]

Since streaming backend support will be available from 8.0(?), plugins built using the proposed changes will crash on 7.x, correct? Would we prefer to maintain separate starters for 7.0 and 8.0, or to have the starter work on both?

I'm leaning towards the latter, have the starter work for >=7.0, and instead add docs and examples for how to add streaming support if your plugin will target 8.0. In other words, I think the code simplification is ok, but I don't think we should add streaming to the starter at this time. By instead focusing efforts on docs and examples, plugin authors of existing plugins can benefit as well.

The docs for backend streaming can probably go in the current guide for streaming data sources. We can also provide a complete plugin example.

Edit: To clarify, the purpose of this repository is to get plugin authors up-and-running quickly. Focus should be on making it easy to understand the most common use case, rather than demonstrating what features are available. We should use plugin examples for that.

[FZambia]

Since streaming backend support will be available from 8.0(?), plugins built using the proposed changes will crash on 7.x, correct?

They should work, just streaming methods won't be called. Don't see a reason for a crash, maybe missing sth?

[FZambia]

To clarify, the purpose of this repository is to get plugin authors up-and-running quickly. Focus should be on making it easy to understand the most common use case, rather than demonstrating what features are available. We should use plugin examples for that.

Makes sense, let's discuss a general approach then (I'll remove streaming parts from here eventually and add a new example to https://github.com/grafana/grafana-plugin-examples but I suppose we want to have a better initial template here too)

[marcusolsson]

I guess another question I have then is whether we expect most if not all future backend plugins to have streaming support, or whether it's considered a feature that not all will use. I'd like for the starter to be a scaffold for a minimal plugin that will be relevant for most if not all who use it. If we consider streaming to be part of those basic set features, then I think it could make sense to add it.

It's better to have new plugin authors get started with something that works, and then have them read up on how to add the next thing, rather than trying to make sense of something they might not need. We want people to get to something that works as fast as possible, preferably having a basic understanding of the code in the same time. Adding more features to the starter works against that goal, so we got to make sure they're worth it.

[FZambia]

I guess another question I have then is whether we expect most if not all future backend plugins to have streaming support, or whether it's considered a feature that not all will use. I'd like for the starter to be a scaffold for a minimal plugin that will be relevant for most if not all who use it. If we consider streaming to be part of those basic set features, then I think it could make sense to add it.

Of course streaming usage will be limited, so definitely agree with point about keeping template minimal here. But we have to come to a template which is straightforward to extend correctly. It took me about 2 hours to realize what's going on here, how to access datasource settings, how to properly cleanup (still not sure how).

[marcusolsson]

But we have to come to a template which is straightforward to extend correctly. It took me about 2 hours to realize what's going on here, how to access datasource settings, how to properly cleanup (still not sure how).

💯

Anything we can do to make this easier to understand will be greatly appreciated, though I'm afraid that a big part of the confusion comes from the APIs themselves. I'm still having trouble following the instance configuration even for my own plugins 😅 . Even tried creating a wrapper, but came up short. Can't put my finger on it, but I think the confusion for me at least is having to keep track of what's considered part of the data source as opposed to the data source instance.

[FZambia]

I think the confusion for me at least is having to keep track of what's considered part of the data source as opposed to the data source instance.

If we want to have some global state and decouple it from instance we may tweak template a bit.

Let's look at what this pr currently suggests:

func main() {
	_ = datasource.Manage(NewSampleDatasource, datasource.ManageOpts{})
}

// NewSampleDatasource creates a new datasource instance.
func NewSampleDatasource(_ backend.DataSourceInstanceSettings) (instancemgmt.Instance, error) {
	return &SampleDatasource{}, nil
}

// SampleDatasource is an example datasource used to scaffold
// new datasource plugins with an backend.
type SampleDatasource struct {
	closeCh chan struct{}
}

func (d *SampleDatasource) Dispose() {
	close(d.closeCh)
}

func (d *SampleDatasource) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {
	return nil, nil
}

I think we can modify it to be sth like this:

func main() {
	ds := NewSampleDatasource()
	_ = datasource.Manage(ds.NewSampleDatasourceInstance, datasource.ManageOpts{})
}

type SampleDatasource struct {
	// Global not-instance specific parts here, created on plugin start.
}

func NewSampleDatasource() *SampleDatasource {
	return &SampleDatasource{}
}

// NewSampleDatasourceInstance creates a new datasource instance.
func (d *SampleDatasource) NewSampleDatasourceInstance(_ backend.DataSourceInstanceSettings) (instancemgmt.Instance, error) {
	return &SampleDatasourceInstance{
		datasource: d, // to access global things
	}, nil
}

type SampleDatasourceInstance struct {
	datasource *SampleDatasource
}

func (d *SampleDatasourceInstance) Dispose() {
	close(d.closeCh)
}

func (d *SampleDatasourceInstance) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {
	return nil, nil
}

[ryantxu]

the purpose of this repository is to get plugin authors up-and-running quickly.

💯 the changes @FZambia proposes here (and in the SDK!) make it much more possible. This embraces the instance manager model and makes it "just work" like it does in the frontend.

I think this is a much more approachable model to promote than the one where people have to manage that themselves.

Is streaming normal? I just hope so :) The goal is to make this a normal common thing that is easy to do. I would argue that it is even more useful for custom plugins since many non-database things can do a good job saying what their current state is.

@marcusolsson anything specific you want to be changed to remove the blocker? It seems not great to want a new streaming example... rather than one well maintained example that shows the APIs (people can easily remove them if they do not want it)

Perhaps we should wrap the UI checkbox for streaming in a version check? in 7.0 you can set it, but it would not actually stream anything.

[marcusolsson]

The only comment I have is about adding the streaming support. Otherwise this looks good!

It seems not great to want a new streaming example... rather than one well maintained example that shows the APIs (people can easily remove them if they do not want it)

I don't see how this is a good argument. It could be used to justify adding just about any feature to the starter. Every feature we add will make it harder for new plugin authors to wrap their heads around it. They way I see it, the starter should be relevant for at least 80% of the people who use it. If instead 80% have to remove said feature, we risk losing newcomers who struggle to figure out things they don't even need.

If you deem streaming to be relevant for most of the people using the starter, then I'm ok with adding it as long as we make it clear that it's only available for 8.0 Edit: ... and that the plugin works for 7.0.

Dismissing for now.

[FZambia]

Since this adds more craft to the starter - what about simply putting Grafana v8+ notice to a switch label? Maybe we can provide this code as part of streaming tutorial doc? But I think we better not deal with versioning in a starter.

[ryantxu]

pushed

[kminehart]

These can be combined, no? Or did you want to have them separated for clarity?

[FZambia]

Ryan fixed this in 6c1c824

[kminehart]

I do appreciate the abstraction of the instancemanager. that was one of the more confusing aspects that I remember when I first started.

I'm not sure which is better, <- time.After(...) or using a time.Ticker. I think I prefer a time.Ticker but that's just me, maybe introducing a new type might not be the best idea.

Looks good to me!

[marefr]

Looks good in general. Don't really have any opinion regarding whether to include streaming or not or simple vs advance. To showcase the streaming feature its good to include it, but could be confusing if developers thinks they need to implement everything such as health check, query data, resource and streaming rather than start with queryData for example. If streaming is going to be part of this I would suggest that CallResource example is included as well.

On a related note. Think we need to attack the main problem soon - the SDK api's does not guide the developer in a good way and there's now many different ways/alternatives to implement a data source and/or certain capabilities. Could we decide on one way/api and introduce a breaking change to remove things we do not want? Not a bad thing to do since this is a result of feedback from many developers.

[FZambia]

developers thinks they need to implement everything such as health check, query data, resource and streaming rather than start with queryData for example. If streaming is going to be part of this I would suggest that CallResource example is included as well.

For now let me add some comments into code which explains that a bit, and yep - adding CallResource sounds like a good thing since we are adding streaming.

[kminehart]

If you're adding CallResource, I think a welcome addition for users experienced in Go would be to use the httpadapter.