Convert adapter utils to middlewares #1118
Conversation
5261aee to
3570f36
Compare
|
|
||
| var once = sync.Once{} | ||
|
|
||
| func metricWrapper(next handlerWrapperFunc) handlerWrapperFunc { |
There was a problem hiding this comment.
this was a bit confusing since the "tracingWrapper" got moved to a middleware in this PR but the rest aren't. I just saw that NewMetricsMiddleware, etc already exist.
There was a problem hiding this comment.
Yes it was added in #1117 as explained in the description. It was added so Grafana could start use it sooner rather than later given this is quite a lot of changes
There was a problem hiding this comment.
LGTM! It looks like you covered everything. It'd be good to have another pair of eyes here, just in case, @wbrowne ?
backend/serve.go
Outdated
| newTenantIDMiddleware(), | ||
| newContextualLoggerMiddleware(), | ||
| NewTracingMiddleware(tracing.DefaultTracer()), | ||
| NewMetricsMiddleware(prometheus.DefaultRegisterer, "grafana", false), |
There was a problem hiding this comment.
I assume the switch to use namespace = "grafana" and subsystem = "plugin" is virtually the same as the pre-existing namespace = "grafana_plugin"
There was a problem hiding this comment.
Yes exactly, but let me double check that 😄
There was a problem hiding this comment.
Used external plugin in local grafana with these changes and browsed the http://localhost:3000/metrics/plugins/<plugin id> and seems to work as expected 🎉
# HELP grafana_plugin_request_total The total amount of plugin requests
# TYPE grafana_plugin_request_total counter
grafana_plugin_request_total{endpoint="queryData",status="ok",status_source="plugin"} 2
…use default middlewares
| if opts.HandlerMiddlewares == nil { | ||
| opts.HandlerMiddlewares = make([]HandlerMiddleware, 0) | ||
| } | ||
|
|
||
| middlewares := defaultHandlerMiddlewares() | ||
| opts.HandlerMiddlewares = append(middlewares, opts.HandlerMiddlewares...) | ||
|
|
||
| pluginOpts, err := GRPCServeOpts(opts) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
@wbrowne since used in experimental/datasourcetest and by some external plugins, e.g. https://github.com/grafana/google-sheets-datasource/blob/fe8de66f07a9920dbec0ed07836d56850e7ed8bc/pkg/googlesheets/datasource_test.go#L11, adding the default middlewares here. Does this make sense? Tests passes now at least.
What this PR does / why we need it:
Follow up from #1117 where I decided to put the convert adapter utils to middlewares in a separate PR. This converts common adapter logic to middlewares that can be reused between the SDK and Grafana.
Which issue(s) this PR fixes:
Ref #1101
Ref #1117
Special notes for your reviewer: