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

API: Permit Cache-Control (browser caching) for datasource resources #62033

Merged
merged 4 commits into from
Feb 2, 2023

Conversation

kylebrandt
Copy link
Contributor

@kylebrandt kylebrandt commented Jan 24, 2023

This will allow Cache-Control trough and not overwrite it if:

  • X-Grafana-Cache header is set
  • Cache-Control is private

The more immediate goal is to allow certain resources in Prometheus to be cached to improve query builder specifically. This is a piece of that.

X-Grafana-Cache is a header that I believe I just made up (there is a X-Grafana-NoCache already). The idea is to make it "opt-in", so this change doesn't unintentionally change other data sources in unanticipated ways.

@kylebrandt kylebrandt added this to the 9.4.0 milestone Jan 27, 2023
@kylebrandt kylebrandt changed the title API: (WIP) Resource cache control API: Permit Cache-Control (browser caching) for datasource resources Jan 27, 2023
@kylebrandt kylebrandt marked this pull request as ready for review January 27, 2023 14:51
@kylebrandt kylebrandt requested a review from a team as a code owner January 27, 2023 14:51
@kylebrandt kylebrandt requested review from sakjur, tolzhabayev, mildwonkey, papagian and marefr and removed request for a team January 27, 2023 14:51
@gtk-grafana gtk-grafana requested review from gtk-grafana and removed request for gtk-grafana January 27, 2023 15:11
@tolzhabayev tolzhabayev removed their request for review January 27, 2023 16:43
Copy link
Member

@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.

LGTM, added a suggestion

pkg/middleware/middleware.go Outdated Show resolved Hide resolved
Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
@kylebrandt kylebrandt modified the milestones: 9.4.0, 9.5.0 Feb 2, 2023
@kylebrandt kylebrandt enabled auto-merge (squash) February 2, 2023 20:20
@kylebrandt kylebrandt merged commit 27d429e into main Feb 2, 2023
@kylebrandt kylebrandt deleted the resourceCacheControl branch February 2, 2023 20:34
ryantxu pushed a commit that referenced this pull request Feb 3, 2023
…62033)

* Start work on allowing certain resources to pass through Cache-Control headers.
---------

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

foundPrivate := false
foundPublic := false
for _, val := range ccHeaderValues {
Copy link
Contributor

Choose a reason for hiding this comment

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

ccHeaderValues is a string of comma separated values, so if the cache control includes a max-age this check will fail.
For example, I would expect to be able to set the following headers in the datasource resource.go:

 Cache-Control: private, max-age=806400
 X-Grafana-Cache: y

and have that pass this check.

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

Successfully merging this pull request may close these issues.

None yet

4 participants