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.

Sign up for GitHub

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

Jump to bottom

InfluxDB: Add support for POST HTTP verb #16690

Merged
merged 1 commit into from
May 2, 2019
Merged

InfluxDB: Add support for POST HTTP verb #16690

merged 1 commit into from
May 2, 2019

Conversation

StephenSorriaux
Copy link
Contributor

What this PR does / why we need it:
A new parameter queryMode is added to the InfluxDB datasource to provide a way to use POST instead of GET when querying the database. This prevents to get any error when querying the database with an heavy request.
Default configuration is kept to GET for backward compatibility. Tests have been added for this new behaviour.

Which issue(s) this PR fixes:
Fixes #10038

Special notes for your reviewer:
None at the moment.

bergquist
bergquist previously requested changes Apr 23, 2019
View reviewed changes
Copy link
Contributor

@bergquist bergquist left a comment

Choose a reason for hiding this comment

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

Thank you for taking time to contribute to Grafana.

This should be implemented backend as well. https://github.com/grafana/grafana/tree/master/pkg/tsdb/influxdb So all requests for a datasource is using the same http verb.

@StephenSorriaux
Copy link
Contributor Author

@bergquist thanks for your review, I clearly overlooked this part. I will add a commit for the backend.

@StephenSorriaux StephenSorriaux changed the title area/datasource: add support for POST HTTP verb for InfluxDB WIP: area/datasource: add support for POST HTTP verb for InfluxDB Apr 23, 2019
@bergquist
Copy link
Contributor

No worries. It's kinda weird that Grafana have backend and frontend implementation ;)

@CLAassistant
Copy link

CLAassistant commented Apr 23, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@StephenSorriaux StephenSorriaux force-pushed the feat/add-support-POST-verb-influxdb branch 2 times, most recently from 002de9d to 00f3f37 Compare April 23, 2019 19:16
@StephenSorriaux StephenSorriaux changed the title WIP: area/datasource: add support for POST HTTP verb for InfluxDB area/datasource: add support for POST HTTP verb for InfluxDB Apr 23, 2019
@StephenSorriaux
Copy link
Contributor Author

@bergquist I am ready for a new review, since the backend has now its changes. :)
Just a question, currently it is possible to curl -XPUT -d '{..."jsonData": {"queryMode": "PUT"}} ....}' /api/datasources/:id without getting any error (PUT should be refused)... That seems wrong, how can I prevent it?

@bergquist bergquist requested a review from aocenas April 30, 2019 11:31
aocenas
aocenas reviewed May 1, 2019
View reviewed changes
Copy link
Member

@aocenas aocenas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the contribution. Here are some minor comments from me. I do not think they would prevent merging of this so let me know if you have time for those and if not we can address them after merging.

pkg/tsdb/influxdb/influxdb_test.go Outdated Show resolved Hide resolved
public/app/plugins/datasource/influxdb/datasource.ts Outdated Show resolved Hide resolved
public/app/plugins/datasource/influxdb/partials/config.html Outdated Show resolved Hide resolved
@aocenas
Copy link
Member

aocenas commented May 1, 2019 •
edited
Loading

@StephenSorriaux

Just a question, currently it is possible to curl -XPUT -d '{..."jsonData": {"queryMode": "PUT"}} ....}' /api/datasources/:id without getting any error (PUT should be refused)... That seems wrong, how can I prevent it?

Yeah we do not have that much backend validation fur such cases. For example for dashboards validation is here so it can be the same for all API updates but also provisioning. So even though validation would be nice I am not sure it makes sense to do it in this PR, as this would need to add some more validation boilerplate and I am not sure if there is consensus in where to actually put the validation (for example I think it makes sense to have single validation logic for all inputs but I think data should be validated as soon as possible on the API boundary). cc @bergquist

@StephenSorriaux StephenSorriaux force-pushed the feat/add-support-POST-verb-influxdb branch from 00f3f37 to 27fd995 Compare May 1, 2019 11:19
@aocenas aocenas mentioned this pull request May 1, 2019
Closed
@StephenSorriaux StephenSorriaux force-pushed the feat/add-support-POST-verb-influxdb branch from 27fd995 to 9487621 Compare May 1, 2019 11:45
@StephenSorriaux
Add support for POST HTTP verb when configuring InfluxDB as a datasou…
0092c23 
…rce.

A new parameter `queryMode` is added to the InfluxDB datasource to provide a way to use POST instead of GET when querying the database. This prevents to get any error when querying the database with an heavy request.
Default configuration is kept to GET for backward compatibility. Tests and documentation have been added for this new behaviour.

Fixes grafana#10038
@StephenSorriaux StephenSorriaux force-pushed the feat/add-support-POST-verb-influxdb branch from 9487621 to 0092c23 Compare May 1, 2019 11:49
@StephenSorriaux
Copy link
Contributor Author

@bergquist @aocenas I am ready for a new review. :)

@aocenas
Copy link
Member

aocenas commented May 2, 2019

Thanks for the changes @StephenSorriaux

@aocenas aocenas changed the title area/datasource: add support for POST HTTP verb for InfluxDB Datasources: add support for POST HTTP verb for InfluxDB May 2, 2019
@aocenas aocenas added this to the 6.2-beta1 milestone May 2, 2019
@aocenas aocenas merged commit 3866839 into grafana:master May 2, 2019
ryantxu added a commit to ryantxu/grafana that referenced this pull request May 2, 2019
@ryantxu
Merge remote-tracking branch 'grafana/master' into react-app-pages
e0cf409 
* grafana/master:
  Datasources: add support for POST HTTP verb for InfluxDB (grafana#16690)
  Add pattern validation in configs (grafana#16837)
  Search: Enable filtering dashboards in search by current folder (grafana#16790)
  FormLabel: allow any rather than just a string for tooltip (grafana#16841)
ryantxu added a commit to ryantxu/grafana that referenced this pull request May 2, 2019
@ryantxu
Merge remote-tracking branch 'grafana/master' into min-max-field
cdd84f8 
* grafana/master:
  Datasources: add support for POST HTTP verb for InfluxDB (grafana#16690)
  Add pattern validation in configs (grafana#16837)
  Search: Enable filtering dashboards in search by current folder (grafana#16790)
ryantxu added a commit to ryantxu/grafana that referenced this pull request May 3, 2019
@ryantxu
Merge remote-tracking branch 'grafana/master' into transforms
3aefc3c 
* grafana/master:
  Chore: Lowered implicit anys limit to 5623
  TestData: Add dashboards to testdata (grafana#16855)
  Input Datasource: convert from angular config to react ConfigEditor (grafana#16856)
  DataSources: minor typescript cleanups and comments (grafana#16860)
  TestDataDatasource: Add config editor (grafana#16861)
  App Plugins: support react pages and tabs (grafana#16586)
  Add Windows MSI generation to build process (grafana#16502)
  Datasources: add support for POST HTTP verb for InfluxDB (grafana#16690)
  Add pattern validation in configs (grafana#16837)
  Search: Enable filtering dashboards in search by current folder (grafana#16790)
ryantxu added a commit to ryantxu/grafana that referenced this pull request May 3, 2019
@ryantxu
Merge remote-tracking branch 'grafana/master' into useQueryRunner-in-…
85c3901 
…MetricPanelCtrl

* grafana/master: (24 commits)
  CSV: escape quotes in toCSV  (grafana#16874)
  Dashboard: Lazy load out of view panels (grafana#15554)
  LDAP: Added reload endpoint for LDAP config (grafana#15470)
  PluginsList: Removed icons and updated snapshots (grafana#16872)
  Panels: Fixed issue with panel type change and data updates (grafana#16871)
  Chore: fix modes for non-executable files (grafana#16864)
  DataSourceSettings: Minor fix for uncontrolled input (grafana#16863)
  Chore: Lowered implicit anys limit to 5623
  TestData: Add dashboards to testdata (grafana#16855)
  Input Datasource: convert from angular config to react ConfigEditor (grafana#16856)
  DataSources: minor typescript cleanups and comments (grafana#16860)
  TestDataDatasource: Add config editor (grafana#16861)
  App Plugins: support react pages and tabs (grafana#16586)
  Add Windows MSI generation to build process (grafana#16502)
  Datasources: add support for POST HTTP verb for InfluxDB (grafana#16690)
  Add pattern validation in configs (grafana#16837)
  Search: Enable filtering dashboards in search by current folder (grafana#16790)
  FormLabel: allow any rather than just a string for tooltip (grafana#16841)
  prometheus: fix regression after adding support for tracing headers (grafana#16829)
  area/circleci: Speed up circleci build process for branches and pr (grafana#16778)
  ...
ryantxu added a commit to ryantxu/grafana that referenced this pull request May 3, 2019
@ryantxu
Merge remote-tracking branch 'grafana/master' into reuse-query2
1b447d7 
* grafana/master: (44 commits)
  CSV: escape quotes in toCSV  (grafana#16874)
  Dashboard: Lazy load out of view panels (grafana#15554)
  LDAP: Added reload endpoint for LDAP config (grafana#15470)
  PluginsList: Removed icons and updated snapshots (grafana#16872)
  Panels: Fixed issue with panel type change and data updates (grafana#16871)
  Chore: fix modes for non-executable files (grafana#16864)
  DataSourceSettings: Minor fix for uncontrolled input (grafana#16863)
  Chore: Lowered implicit anys limit to 5623
  TestData: Add dashboards to testdata (grafana#16855)
  Input Datasource: convert from angular config to react ConfigEditor (grafana#16856)
  DataSources: minor typescript cleanups and comments (grafana#16860)
  TestDataDatasource: Add config editor (grafana#16861)
  App Plugins: support react pages and tabs (grafana#16586)
  Add Windows MSI generation to build process (grafana#16502)
  Datasources: add support for POST HTTP verb for InfluxDB (grafana#16690)
  Add pattern validation in configs (grafana#16837)
  Search: Enable filtering dashboards in search by current folder (grafana#16790)
  FormLabel: allow any rather than just a string for tooltip (grafana#16841)
  prometheus: fix regression after adding support for tracing headers (grafana#16829)
  area/circleci: Speed up circleci build process for branches and pr (grafana#16778)
  ...
@marefr marefr changed the title Datasources: add support for POST HTTP verb for InfluxDB InfluxDB: Add support for POST HTTP verb May 7, 2019
@SibrenVasse SibrenVasse mentioned this pull request May 7, 2019
Closed
@StephenSorriaux StephenSorriaux deleted the feat/add-support-POST-verb-influxdb branch May 9, 2019 07:59
@ying-jeanne ying-jeanne added the pr/external This PR is from external contributor label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aocenas aocenas aocenas approved these changes

@bergquist bergquist bergquist left review comments

Assignees
No one assigned
Labels
add to changelog area/backend area/datasource datasource/InfluxDB pr/external This PR is from external contributor
Projects
None yet
Milestone
6.2-beta1
Development

Successfully merging this pull request may close these issues.

Use POST Requests to query data from influxdb
5 participants
@StephenSorriaux @bergquist @CLAassistant @aocenas @ying-jeanne