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

Make "instance" a core SDK idea #105

Closed
ryantxu opened this issue Mar 24, 2020 · 4 comments · Fixed by #149
Closed

Make "instance" a core SDK idea #105

ryantxu opened this issue Mar 24, 2020 · 4 comments · Fixed by #149

Comments

@ryantxu
Copy link
Member

ryantxu commented Mar 24, 2020

on the frontend, a plugin instance is managed independently -- on the backend each request needs to sort out which instance it is and does not have a clear place to initalize/destroy config changes

Perhaps something like:

type InstanceRecycler interface {
	Recycle() error
}

type PluginInstance struct {
	CheckHealthHandler  CheckHealthHandler
	CallResourceHandler CallResourceHandler
	QueryDataHandler    QueryDataHandler
	Recycler            InstanceRecycler
}

type PluginHandler interface {
	CreateAppPluginInstance(ctx context.Context, config PluginConfig) (*PluginInstance, error)
	CreateDataSourceInstance(ctx context.Context, config PluginConfig) (*PluginInstance, error)
}

The SDK would then manage calls so that:

  • Create{DataSource|AppPlugin}Instance was called when appropriate
  • the correct instance is called for each health|query|resource|... request
  • when the configs change (based on the time parameter) the Recycle function is called

Thoughts? This is related to #99 -- and hopefully explains why I think this solves @bergquist's issue with the healthcheck clarity

@marefr
Copy link
Member

marefr commented Mar 31, 2020

I'm not sure this makes things easier since the handlers are still of the same contract. Regarding CreateAppPluginInstance/CreateDataSourceInstance, either you would have an app plugin or a data source plugin. Also in the frontend you have constructors so I don't think doing a 1-1 mapping is optimal. But good input and gives me some ideas.

@kylebrandt
Copy link
Contributor

kylebrandt commented Apr 1, 2020

So thinking about this, I think some practical plugin building problem something like this should help with:

An instance of something, often, will have its own API client to something else in the backend - amc azuremagic/client ... client := amc.NewClient(stuff from plugin/datasource config). We would want to reuse that API client between requests, on a per instance basis.

So for example, a SQL backend plugin might have multiple instances configured in Grafana. Each instance probably holds something like a connection string. So a client gets created from the connection string, and it is far more efficient to reuse that client and underlying connections when possible.

Also caching would be a per instance thing.

@marefr
Copy link
Member

marefr commented Apr 2, 2020

I'm not saying no the concept of caching http clients/connections. But I think I prefer a solution similar to what we have in Grafana backend.

@bergquist
Copy link
Contributor

bergquist commented Apr 6, 2020

Frontend plugins can assume that all datasources talk over HTTP so it's a little bit easier to construct instances. For backend plugins, the plugin author would have to implement a constructor function to create the instance since we don't know what kind of client is returning data filereader/db client/http client. Clients will behave differently.

As I see it we either create helpers that can help with cache/client management or we require the plugin author to pass a constructor function at serve. I think the helper route is simpler to reason about and debug. Passing a ctor function at serve sounds like fun code to write and could be quite elegant in some cases.

My issue with health check is that its that we use the same endpoint for health checking the entire plugin and health checking a configured datasource.

marefr added a commit that referenced this issue May 5, 2020
Removes the experimental package.
Adds new datasource package for serving datasource plugins 
with optional instance management support .
New instancemgmt package for managing caching and 
disposal of instances.
Moves resource response sender utils to resource package
Moves gRPC settings to its own struct.
Add convenience handler funcs for query data, call resource 
and check health handlers.
Replace QueryTypeHandlerFunc with backend.QueryDataHandlerFunc.
Moves query type mux example to separate test package.

Closes #105
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants