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

Add silent option to backend requests #11678

Merged
merged 3 commits into from Apr 25, 2018
Merged

Conversation

davkal
Copy link
Contributor

@davkal davkal commented Apr 20, 2018

  • When set to true, the silent option for backend_srv requests
    suppresses all event emitters that are triggered when the response is
    received.
  • Added metadataRequest() to the Prometheus datasource to support
    requests that are not triggered by the user, e.g., for tab completion.
    metadataRequest() sets the silent option.
  • Migrated all non-timeseries queries of the Prometheus datasource to
    use metadataRequest().
  • Combined positional args of _request into options dict.

Fixes #11673

* When set to `true`, the `silent` option for backend_srv requests
 suppresses all event emitters that are triggered when the response is
received.
* Added `helperRequest()` to the Prometheus datasource to support
 requests that are not triggered by the user, e.g., for tab completion.
`helperRequest()` sets the `silent` option.
* Migrated all non-timeseries queries of the Prometheus datasource to
 use `helperRequest()`.

Fixes #11673
@davkal davkal requested a review from marefr April 20, 2018 13:38
@@ -81,6 +81,20 @@ export class PrometheusDatasource {
return this.backendSrv.datasourceRequest(options);
}

// Use this for tab completion features, wont publish response to other components
helperRequest(url) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can come up with a better name :)

Copy link
Member

Choose a reason for hiding this comment

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

Agree with Carl. How about metadataRequest?

@marefr marefr self-assigned this Apr 23, 2018
@@ -81,6 +81,20 @@ export class PrometheusDatasource {
return this.backendSrv.datasourceRequest(options);
}

// Use this for tab completion features, wont publish response to other components
helperRequest(url) {
Copy link
Member

Choose a reason for hiding this comment

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

Agree with Carl. How about metadataRequest?

silent: true,
};

if (this.basicAuth || this.withCredentials) {
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit unfortunate to multiply this logic and possibly easy to miss if making a change to the _request function. Would it be possible to let this function reuse the _request function? Maybe by introducing a new argument at the end of _request function.

From review feedback:

* s/helper/metadata
* combined positional args to _request into options dict
* metadataRequest reuses _request()
* moved consumption of this.httpMethod into _request, can be overwritten
 in options due to spread-after
@davkal
Copy link
Contributor Author

davkal commented Apr 24, 2018

Thanks for the feedback @marefr PTAL

var options: any = {
url: this.url + url,
method: method,
requestId: requestId,
method: this.httpMethod,
Copy link
Member

Choose a reason for hiding this comment

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

This does not work for metric find queries when using Prometheus >= v2.1.0 and HTTP method is configured to POST. When editing a template variable query I receive "Template variables could not be initialized: Cannot read property 'jquery' of null".

I think all meta data requests should always use GET to resolve this problem. Some tests of this would be nice for the _request method and/or metric find queries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

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.

Great 👍

@davkal davkal merged commit c99d6bd into master Apr 25, 2018
@davkal davkal deleted the davkal/11673-silent-response branch April 25, 2018 07:46
@marefr marefr modified the milestones: 5.2, 5.1 Apr 25, 2018
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.

None yet

4 participants