Block requests to ui_metric API if telemetry is disabled #35268
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cjcenizal
added
Team:Core
|
Pinging @elastic/kibana-platform |
cjcenizal
force-pushed
the
uim/disable-when-opted-out
branch
from
a8a3c09
to
79dca6a
Compare
- Export track helper method from ui_metric app. - Remove createUiMetricUri helper.
cjcenizal
force-pushed
the
uim/disable-when-opted-out
branch
from
79dca6a
to
c981758
Compare
cjcenizal
commented
Apr 17, 2019
| const currentSignature = response.headers('kbn-xpack-sig'); | ||
| const cachedSignature = xpackInfoSignature.get(); | ||
| // If another interceptor has rejected a request, then headers will be undefined. | ||
| if (response.headers) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -65,7 +65,9 @@ module.config(($httpProvider) => { | |||
|
|
|||
| function interceptorFactory(responseHandler) { | |||
| return function interceptor(response) { | |||
| if (!isUnauthenticated && !isSystemApiRequest(response.config) && sessionTimeout !== null) { | |||
| // If another interceptor has rejected a request, then config will be undefined. | |||
| const isSystemRequest = response.config ? isSystemApiRequest(response.config) : false; | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
💔 Build Failed |
|
CC @tylersmalley as I believe this is the first instance of statically importing OSS code into production X-Pack code, as enabled by #32722. |
💔 Build Failed |
kobelb
reviewed
Apr 18, 2019
| }); | ||
|
|
||
| module.config(($httpProvider) => { | ||
| $httpProvider.interceptors.push(($q) => { |
There was a problem hiding this comment.
Why are we using an interceptor to accomplish this? Would it be possible to just not make the calls when telemetry is disabled instead of trying to block them?
There was a problem hiding this comment.
Haha, yes! I implemented the interceptor solution before creating the common track method, so completely overlooked the obvious. I'll update the code to just perform the check there.
…rceptors. - Add support for an array of metric types. - Update README.
cjcenizal
force-pushed
the
uim/disable-when-opted-out
branch
from
19100f5
to
d084eaa
Compare
|
Thanks @kobelb, I've applied your suggestion. |
kobelb
approved these changes
Apr 18, 2019
💔 Build Failed |
💚 Build Succeeded |
… containing a colon.
💔 Build Failed |
💔 Build Failed |
cjcenizal
force-pushed
the
uim/disable-when-opted-out
branch
from
0b60e90
to
80ababd
Compare
💚 Build Succeeded |
sebelga
approved these changes
Apr 23, 2019
|
Thanks, Seb! I'm handing this off to @Bamieh, who is on the Platform team, so I'll let him decide how to handle integration with the NP. |
Bamieh
approved these changes
Apr 25, 2019
| to worry about exceeding the 1000-field soft limit in Elasticsearch. | ||
|
|
||
| // TODO: Clarify that colons are NOT allowed in metric names or app types possibly reject request in trackUiMetric; disallow commas too? |
There was a problem hiding this comment.
I think this todo is no longer needed as you have already addressed it on line 37
💚 Build Succeeded |
cjcenizal
added a commit
to cjcenizal/kibana
that referenced
this pull request
Apr 25, 2019
* Block requests to ui_metric API if telemetry is disabled - Export trackUiMetric helper method from ui_metric app. - Remove createUiMetricUri helper. - Add support for an array of metric types. - Update README. * Throw error if trackUiMetric is called with a string containing a colon.
cjcenizal
added a commit
that referenced
this pull request
Apr 25, 2019
…5629) * Block requests to ui_metric API if telemetry is disabled - Export trackUiMetric helper method from ui_metric app. - Remove createUiMetricUri helper. - Add support for an array of metric types. - Update README. * Throw error if trackUiMetric is called with a string containing a colon.
lizozom
pushed a commit
to lizozom/kibana
that referenced
this pull request
Apr 29, 2019
* Block requests to ui_metric API if telemetry is disabled - Export trackUiMetric helper method from ui_metric app. - Remove createUiMetricUri helper. - Add support for an array of metric types. - Update README. * Throw error if trackUiMetric is called with a string containing a colon.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #34387
We need to prevent requests to the
ui_metricAPI from being sent if telemetry is disabled; otherwise users might assume we're still gathering telemetry. This change adds an interceptor to the$httpservice which blocks the request, and also introduces some hackery to ensure all requests to the API go through$httpand that blocked requests don't bubble up warnings to the console.To test:
api/ui_metric.I've also tested this by creating a build of Kibana and checking that the
ui_metricrequests were and were not being set with/without telemetry, just to verify that X-Pack was still consuming the OSS code as expected.NOTE: A bug currently prevents users from toggling telemetry under Advanced Settings, so you may need to test this by clearing out and restarting your cluster. The bug will be fixed by #35250.
This PR also throws an error if the user calls
trackUiMetricwith an app name or metric type that contains a colon. I expect people to test new metrics by performing an action and verifying it in the/statsoutput, at which point they'll see this error.