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

[query] Allow GET + POST for read APIs #2055

Merged
merged 3 commits into from
Dec 3, 2019

Conversation

schallert
Copy link
Collaborator

@schallert schallert commented Dec 3, 2019

What this PR does / why we need it: Allow POST requests in coordinator read APIs. Supports common use cases such as Grafana datasources that use POST.

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

Allow query requests to be GET or POST

Does this PR require updating code package or user-facing documentation?:


Copy link
Collaborator

@robskillington robskillington left a comment

Choose a reason for hiding this comment

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

LGTM

@schallert schallert force-pushed the schallert/prom_read_get_post branch from 4da6682 to acbd3e7 Compare December 3, 2019 06:48
@codecov
Copy link

codecov bot commented Dec 3, 2019

Codecov Report

Merging #2055 into master will increase coverage by 7%.
The diff coverage is 33.3%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2055     +/-   ##
========================================
+ Coverage    64.8%   71.9%     +7%     
========================================
  Files        1002    1009      +7     
  Lines       86353   86960    +607     
========================================
+ Hits        56042   62603   +6561     
+ Misses      26196   20130   -6066     
- Partials     4115    4227    +112
Flag Coverage Δ
#aggregator 71% <ø> (+11.3%) ⬆️
#cluster 81.7% <ø> (-1.3%) ⬇️
#collector 45.4% <ø> (-9.5%) ⬇️
#dbnode 79.5% <ø> (+20.7%) ⬆️
#m3em 62.9% <ø> (+14%) ⬆️
#m3ninx 73.9% <ø> (+22.4%) ⬆️
#m3nsch 51.1% <ø> (-26.9%) ⬇️
#metrics 17.7% <ø> (ø) ⬆️
#msg 74.7% <ø> (+0.4%) ⬆️
#query 68.5% <33.3%> (+35.2%) ⬆️
#x 48.2% <ø> (-30.3%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c44f1d...acbd3e7. Read the comment docs.

// PromReadInstantHTTPMethod is the HTTP method used with this resource.
PromReadInstantHTTPMethod = http.MethodGet
var (
// PromReadInstantHTTPMethods is the valid HTTP methods used with this
Copy link
Collaborator

Choose a reason for hiding this comment

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

meganit: PromReadInstantHTTPMethods are the valid HTTP methods...

// TODO: Move to config
initialBlockAlloc = 10
)

var (
// PromReadHTTPMethods is the valid HTTP methods used with this
// resource.
PromReadHTTPMethods = []string{
Copy link
Collaborator

Choose a reason for hiding this comment

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

meganit: PromReadHTTPMethods are the valid HTTP methods...

req.Header.Add("Content-Type", "application/x-www-form-urlencoded")

r, err := testParseParams(req)
require.Nil(t, err, "unable to parse request")
Copy link
Collaborator

Choose a reason for hiding this comment

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

meganit: require.NoError gives slightly more context

Copy link
Collaborator

@arnikola arnikola left a comment

Choose a reason for hiding this comment

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

LGTM, couple of very small nits

@robskillington
Copy link
Collaborator

Going to merge this in, @arnikola will take up the nits as followups

@robskillington robskillington merged commit f47d350 into master Dec 3, 2019
@robskillington robskillington deleted the schallert/prom_read_get_post branch December 3, 2019 15:26
arnikola added a commit that referenced this pull request Dec 3, 2019
arnikola added a commit that referenced this pull request Dec 10, 2019
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.

3 participants