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

(prometheus) prevent error to use $__interval_ms in query #12533

Merged
merged 3 commits into from Jul 11, 2018

Conversation

Projects
None yet
4 participants
@mtanda
Copy link
Collaborator

commented Jul 9, 2018

$__interval_ms type is integer. It cause error.

@torkelo

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

Can you rebase this on latest master?

@davkal

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2018

This function could use a comment and a test. Currently I have no idea why it's there and what input/output is typical.

@mtanda mtanda force-pushed the mtanda:prom_interval_ms branch from 80178d9 to 551bd41 Jul 9, 2018

@torkelo
Copy link
Member

left a comment

thanks @mtanda . A test for this and maybe a comment what this function does. Should this function not always return a string?

@mtanda mtanda force-pushed the mtanda:prom_interval_ms branch from 551bd41 to 5090697 Jul 9, 2018

@mtanda

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 9, 2018

Sorry, I'm not sure what I should do.

There are tests for prometheusRegularEscape().
https://github.com/grafana/grafana/blob/v5.2.1/public/app/plugins/datasource/prometheus/specs/datasource.jest.ts#L168-L178

I think prometheusRegularEscape() should return string type.
So I change the code, convert number to string in caller side.

@davkal

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2018

@mtanda we would like to use opportunities where an error was encountered to extend our unit tests.
In this instance it happened in interpolateQueryExpr. Could we ask you to please add a test suite for interpolateQueryExpr() inside datasource.jest.ts as part of this patch? Ideally it would contain a test that would fail before your fix (and pass with the fix).

@mtanda mtanda force-pushed the mtanda:prom_interval_ms branch from 5090697 to a1beaf3 Jul 10, 2018

@mtanda

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2018

I understand what you need. I add a test case for failure pattern.

@@ -103,6 +103,10 @@ export class PrometheusDatasource {
interpolateQueryExpr(value, variable, defaultFormatFn) {
// if no multi or include all do not regexEscape
if (!variable.multi && !variable.includeAll) {
if (typeof value === 'number') {
// $__interval_ms type is number, convert to string here
value = value.toString();

This comment has been minimized.

Copy link
@torkelo

torkelo Jul 10, 2018

Member

not sure this is the correct place to solve this, maybe the root issue should be fixed instead? wonder if allowing $__interval_ms to have number value is not correct but it should be turned into string when it is save in scopedVars (in metrics_ctrl & prometheus datasource.ts)

This comment has been minimized.

Copy link
@mtanda

mtanda Jul 10, 2018

Author Collaborator

I little hesitate to break the current behavior, but I agree the data type should be string.

@torkelo torkelo merged commit 18a8290 into grafana:master Jul 11, 2018

8 checks passed

ci/circleci: build-all Your tests passed on CircleCI!
Details
ci/circleci: codespell Your tests passed on CircleCI!
Details
ci/circleci: gometalinter Your tests passed on CircleCI!
Details
ci/circleci: mysql-integration-test Your tests passed on CircleCI!
Details
ci/circleci: postgres-integration-test Your tests passed on CircleCI!
Details
ci/circleci: test-backend Your tests passed on CircleCI!
Details
ci/circleci: test-frontend Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

ryantxu added a commit to ryantxu/grafana that referenced this pull request Jul 12, 2018

Merge remote-tracking branch 'grafana/master'
* grafana/master: (29 commits)
  skip backend request if extended statistics is invalid. (grafana#12495)
  Refactor team pages to react & design change (grafana#12574)
  (prometheus) prevent error to use $__interval_ms in query (grafana#12533)
  fix: folder picker did not notify parent that the initial folder had been changed, fixes grafana#12543 (grafana#12554)
  Add support for skipping variable value in URL, fixes grafana#12174 (grafana#12541)
  Don't build-all for PRs
  fix: requests/sec instead of requets (grafana#12557)
  Add folder name to dashboard title (grafana#12545)
  Fix css loading in plugins (grafana#12573)
  Add new sequential color scales
  move go vet out of scripts and fixing warning (grafana#12552)
  Cleanup and remove some jest.fn()
  Remove irrelevant tests and templateSrv stub
  Update CHANGELOG.md
  fix diff and percent_diff (grafana#12515)
  Update lodash/moment version (grafana#12532)
  Add mock to test files
  Create new instance in beforeEach
  Remove comments
  Karma to Jest: Cloudwatch datasource
  ...

ryantxu added a commit to ryantxu/grafana that referenced this pull request Jul 12, 2018

Merge remote-tracking branch 'grafana/master' into panel-visibility-flag
* grafana/master: (30 commits)
  skip backend request if extended statistics is invalid. (grafana#12495)
  Refactor team pages to react & design change (grafana#12574)
  (prometheus) prevent error to use $__interval_ms in query (grafana#12533)
  fix: folder picker did not notify parent that the initial folder had been changed, fixes grafana#12543 (grafana#12554)
  Add support for skipping variable value in URL, fixes grafana#12174 (grafana#12541)
  Don't build-all for PRs
  fix: requests/sec instead of requets (grafana#12557)
  Add folder name to dashboard title (grafana#12545)
  Fix css loading in plugins (grafana#12573)
  Add new sequential color scales
  move go vet out of scripts and fixing warning (grafana#12552)
  Cleanup and remove some jest.fn()
  Remove irrelevant tests and templateSrv stub
  Update CHANGELOG.md
  fix diff and percent_diff (grafana#12515)
  Update lodash/moment version (grafana#12532)
  Tabs to spaces in tslint (grafana#12529)
  Add mock to test files
  Create new instance in beforeEach
  Remove comments
  ...

ryantxu added a commit to NatelEnergy/grafana that referenced this pull request Jul 12, 2018

Merge remote-tracking branch 'grafana/master'
* grafana/master: (30 commits)
  skip backend request if extended statistics is invalid. (grafana#12495)
  Refactor team pages to react & design change (grafana#12574)
  (prometheus) prevent error to use $__interval_ms in query (grafana#12533)
  fix: folder picker did not notify parent that the initial folder had been changed, fixes grafana#12543 (grafana#12554)
  Add support for skipping variable value in URL, fixes grafana#12174 (grafana#12541)
  Don't build-all for PRs
  fix: requests/sec instead of requets (grafana#12557)
  Add folder name to dashboard title (grafana#12545)
  Fix css loading in plugins (grafana#12573)
  Add new sequential color scales
  move go vet out of scripts and fixing warning (grafana#12552)
  Cleanup and remove some jest.fn()
  Remove irrelevant tests and templateSrv stub
  Update CHANGELOG.md
  fix diff and percent_diff (grafana#12515)
  Update lodash/moment version (grafana#12532)
  Tabs to spaces in tslint (grafana#12529)
  Add mock to test files
  Create new instance in beforeEach
  Remove comments
  ...

@torkelo torkelo added this to the 5.2.2 milestone Jul 13, 2018

marefr added a commit that referenced this pull request Jul 24, 2018

marefr added a commit that referenced this pull request Jul 25, 2018

(prometheus) prevent error to use $__interval_ms in query (#12533)
* prevent error to use $__interval_ms in query

* add test

* prevent error to use $__interval_ms in query

(cherry picked from commit 18a8290)

marefr added a commit that referenced this pull request Jul 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.