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

automatically debug-log upstream http-response-sizes #1054

Closed
wants to merge 1 commit into from

Conversation

gabor
Copy link
Contributor

@gabor gabor commented Aug 19, 2024

work in progress.

@gabor gabor force-pushed the gabor/log-response-info branch from b4f83e8 to dbedd92 Compare August 22, 2024 14:08
@gabor
Copy link
Contributor Author

gabor commented Aug 22, 2024

@wbrowne @marefr could you please take a look at this draft? it's based on our discussion about logging "upstream" response-sizes... this is not polished yet, i'd just like to know if i'm going in the right direction.. notes:

  • we could alternatively generate metrics instead of logs, but i'm not sure it's really possible to produce metrics from plugin-sdk-go.. logging seems easier
  • my first idea was to create this as a middleware in the default-middleware array, but when i checked this, it seemed that if you configure the http-client with your own middlewares, then you have to manually append the default-middlewares... the current approach seemed more robust
  • i'm checking for a label to be sure this is a datasource-request, plugin-sdk-go-http-clients are also used in other situations where we do not want to do this logging
  • as i said, no polish yet, variable names may be inconsistent etc, will fix that later

in general, WDYT? thanks!

Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

I think this make sense with some minor changes.

After reviewing the code I just realized that we already have something like this in core grafana, see https://github.com/grafana/grafana/blob/main/pkg/infra/httpclient/count_bytes_reader.go and https://github.com/grafana/grafana/blob/ddee95cb6d737ba845380c89456ede9998e12f48/pkg/infra/httpclient/httpclientprovider/datasource_metrics_middleware.go#L115-L119. So I think it would make sense to move the count_bytes_reader.go to the SDK instead replacing your ReportSizeReader

backend/httpclient/report_size_reader.go Outdated Show resolved Hide resolved
Comment on lines +124 to +126
if hasDatasourceTypeLabel {
rt = newReportSizeRoundtripper(rt)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could put this logic in the middleware as well, see this example to get a hold of the options

return NamedMiddlewareFunc(BasicAuthenticationMiddlewareName, func(opts Options, next http.RoundTripper) http.RoundTripper {
. And I think then adding the middleware via DefaultMiddlewares() would allow both
providerOpts.Middlewares = DefaultMiddlewares()
and http_client.go to support this.

One important thing is that this middleware should probably/maybe execute after the https://github.com/grafana/grafana-plugin-sdk-go/blob/main/backend/httpclient/max_bytes_reader.go so if a limit is in place your middleware isn't continuing to read the response to the end 🤷

One thing to consider might be to somehow share the response size reader between limit and reporting middlewares, but maybe something for the future.

@gabor gabor force-pushed the gabor/log-response-info branch from dbedd92 to 750bca5 Compare August 30, 2024 08:18
@gabor
Copy link
Contributor Author

gabor commented Nov 7, 2024

this was achieved by #1130

@gabor gabor closed this Nov 7, 2024
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 this pull request may close these issues.

2 participants