[Infra UI] Fix Metrics Explorer UI and Server Exceptions #39368

simianhacker · 2019-06-20T18:28:38Z

Summary

This PR fixed bugs introduced in #39090 and #39049 that slipped through. The first one only showed up if you loaded the Metrics Explorer without any data. The second one showed up once the first one was fixed and I just couldn't leave it alone because it was an easy fix.

To test this:

Load everything in master
See the metrics explorer fail along with a metrics detail page
Load this PR
Everything should be working as expected

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~This was checked for cross-browser compatibility, including a check against IE11~~
~~Any text added follows EUI's writing guidelines, uses sentence case text and includes [i18n support]~~(https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
~~Documentation was added for features that require explanation or tutorials~~
~~Unit or functional tests were updated or added to match the most common scenarios~~
~~This was checked for keyboard-only and screenreader accessibility~~

elasticmachine · 2019-06-20T18:28:41Z

Pinging @elastic/infra-logs-ui

…xplorer-timestamp-exception

elasticmachine · 2019-06-20T19:34:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…xplorer-timestamp-exception

elasticmachine · 2019-06-21T00:00:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

weltenwort

Not sure if it is related, but I'm seeing this when loading the Metrics Explorer without metricbeat indices:

TypeError: Cannot read property 'series' of undefined
    at series (kibana/x-pack/legacy/plugins/infra/server/routes/metrics_explorer/lib/populate_series_with_tsvb_data.ts:75:41)
    at process._tickCallback (internal/process/next_tick.js:68:7)

x-pack/legacy/plugins/infra/public/components/metrics_explorer/chart.tsx

…xplorer-timestamp-exception

elasticmachine · 2019-06-21T14:51:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

simianhacker · 2019-06-21T15:13:01Z

@weltenwort I was able to reproduce the exception when the Metricbeat indices where missing. I also added a test to ensure both group by and non group by requests work with the missing indices.

elasticmachine · 2019-06-21T15:22:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-21T16:19:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

simianhacker · 2019-06-21T16:52:54Z

retest

weltenwort

That seems to do it. I checked that the tests would have caught the status code 500 without the latest fix. Great that you added them 👍

I left two questions below, but they are no merge blockers.

weltenwort · 2019-06-21T17:34:45Z

x-pack/legacy/plugins/infra/server/routes/metrics_explorer/index.ts

@@ -30,6 +30,9 @@ export const initMetricExplorerRoute = (libs: InfraBackendLibs) => {
        const options = req.payload;
        // First we get the groupings from a composite aggregation
        const response = await getGroupings(search, options);
+        if (response.series.length === 0) {


If response.series is an array of length 0, the map operation below would be a no-op. Why do we need to explicitly handle that case here?

I think I put that in when I was looking at the group by use case. I can remove it.

FYI: I'm going to remove this, merge master, and push to see how CI does.

weltenwort · 2019-06-21T17:36:36Z

...ck/legacy/plugins/infra/server/routes/metrics_explorer/lib/populate_series_with_tsvb_data.ts

@@ -58,6 +58,15 @@ export const populateSeriesWithTSVBData = (
  // Get TSVB results using the model, timerange and filters
  const tsvbResults = await framework.makeTSVBRequest(req, model, timerange, filters);

+  // If there is no data `custom` will not exist.
+  if (!tsvbResults.custom) {


Out of curiosity, why is this called custom? It suggests there's something predefined that we override?

It's just a known attribute, it could be anything we want it to be.

elasticmachine · 2019-06-21T18:04:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

simianhacker · 2019-06-21T19:47:45Z

retest

elasticmachine · 2019-06-21T20:49:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

skh

Apologies to chime in sideways here -- has this been tested with spaces?

skh · 2019-06-24T10:43:39Z

x-pack/legacy/plugins/infra/server/lib/adapters/framework/kibana_framework_adapter.ts

@@ -157,6 +157,8 @@ export class InfraKibanaBackendFrameworkAdapter implements InfraBackendFramework
        url = `/s/${spaceId}${url}`;
      }
    }
+    const basePath = internalRequest.getBasePath();
+    url = `${basePath}${url}`;


Has this been tested with spaces? If I remember correctly, basePath contains the spaces part already -- see the discussion in #36765

good point - I don't think I checked that during my review

@skh @weltenwort that other discussion seems to imply that this shouldn't work even without spaces, if there's a base path (because server.inject() puts the URL after the basepath, so you'd get the basepath in the full URL twice I think?) -- if this does work without spaces, seems like we can use this instead of getting the space ID in other places in this file?

Wasn't the cause for this bug the fact the behavior of server.inject changed to no include the base path since #39049? 🤔

In that case, it sounds like we can simplify a lot of stuff in this file 🤞@legrego from the other discussion said:

can you use internalRequest.getBasePath() instead of deriving a space-aware URL yourself?

If that works, this should become something like

let url = `${internalRequest.getBasePath()}/api/metrics/vis/data`;
and then you can remove the check for the spaces plugin altogether.

Verifying the spaces use-case now :)

Fixed here: b0a8314

x-pack/legacy/plugins/infra/public/components/metrics_explorer/chart.tsx

…rer-timestamp-exception

elasticmachine · 2019-06-25T14:24:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-25T15:12:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

skh

Thanks for checking with spaces.

skh · 2019-06-25T15:28:08Z

x-pack/legacy/plugins/infra/server/lib/adapters/framework/kibana_framework_adapter.ts

    const basePath = internalRequest.getBasePath();
-    url = `${basePath}${url}`;
+    const url = `${basePath}/api/metrics/vis/data`;


jasonrhodes · 2019-06-25T15:31:30Z

retest

elasticmachine · 2019-06-25T16:40:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…rom PR's (elastic#39368) * Fixing exceptions thrown by missing data and missing basePath * Moving dateFormatter to use useMemo instead of useCallback * Improve no-data handling * Adding tests for no-data scenerio * Return empty series when TSVB returns an empty response; update tests * Removes unnecessary early return * Fixes spaces regression by removing custom space detection

…rom PR's (#39368) (#39830) * Fixing exceptions thrown by missing data and missing basePath * Moving dateFormatter to use useMemo instead of useCallback * Improve no-data handling * Adding tests for no-data scenerio * Return empty series when TSVB returns an empty response; update tests * Removes unnecessary early return * Fixes spaces regression by removing custom space detection

elasticmachine · 2019-07-05T15:15:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Fixing exceptions thrown by missing data and missing basePath

789ae65

simianhacker requested a review from a team June 20, 2019 18:28

simianhacker self-assigned this Jun 20, 2019

Merge branch 'master' of github.com:elastic/kibana into fix-metrics-e…

9270bee

…xplorer-timestamp-exception

Merge branch 'master' of github.com:elastic/kibana into fix-metrics-e…

327ba56

…xplorer-timestamp-exception

weltenwort requested review from weltenwort and removed request for a team June 21, 2019 09:42

weltenwort requested changes Jun 21, 2019

View reviewed changes

x-pack/legacy/plugins/infra/public/components/metrics_explorer/chart.tsx Outdated Show resolved Hide resolved

Moving dateFormatter to use useMemo instead of useCallback

2a5bc08

simianhacker requested a review from a team as a code owner June 21, 2019 13:37

Merge branch 'master' of github.com:elastic/kibana into fix-metrics-e…

34581e2

…xplorer-timestamp-exception

simianhacker mentioned this pull request Jun 21, 2019

[Infra UI] Improve Metrics Explorer missing data handling #39436

Closed

7 tasks

simianhacker added 2 commits June 21, 2019 06:47

Improve no-data handling

c2ab4a6

Adding tests for no-data scenerio

ce4b99d

Return empty series when TSVB returns an empty response; update tests

228600d

simianhacker requested a review from weltenwort June 21, 2019 15:11

weltenwort approved these changes Jun 21, 2019

View reviewed changes

skh suggested changes Jun 24, 2019

View reviewed changes

jasonrhodes reviewed Jun 24, 2019

View reviewed changes

x-pack/legacy/plugins/infra/public/components/metrics_explorer/chart.tsx Show resolved Hide resolved

jasonrhodes mentioned this pull request Jun 25, 2019

Defaults metrics explorer to "document count" instead of "average" #39553

Closed

jasonrhodes added 3 commits June 25, 2019 09:08

Merge remote-tracking branch 'upstream/master' into fix-metrics-explo…

bc30ee9

…rer-timestamp-exception

Removes unnecessary early return

d5e4068

Fixes spaces regression by removing custom space detection

b0a8314

skh approved these changes Jun 25, 2019

View reviewed changes

jasonrhodes merged commit 16ae71d into elastic:master Jun 25, 2019

tbragin mentioned this pull request Jun 27, 2019

[Infra UI] Meta-issue for Metrics Explorer GA #39615

Closed

12 tasks

jasonrhodes mentioned this pull request Jun 27, 2019

[7.x] [Infra UI] Fix Metrics Explorer UI and Server Exceptions introduced from PR's (#39368) #39830

Merged

jasonrhodes changed the title ~~[Infra UI] Fix Metrics Explorer UI and Server Exceptions introduced from PR's~~ [Infra UI] Fix Metrics Explorer UI and Server Exceptions Jul 5, 2019

jasonrhodes added the in-test-plan label Jul 5, 2019

simianhacker deleted the fix-metrics-explorer-timestamp-exception branch April 17, 2024 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Infra UI] Fix Metrics Explorer UI and Server Exceptions #39368

[Infra UI] Fix Metrics Explorer UI and Server Exceptions #39368

simianhacker commented Jun 20, 2019

elasticmachine commented Jun 20, 2019

elasticmachine commented Jun 20, 2019

elasticmachine commented Jun 21, 2019

weltenwort left a comment

elasticmachine commented Jun 21, 2019

simianhacker commented Jun 21, 2019

elasticmachine commented Jun 21, 2019

elasticmachine commented Jun 21, 2019

simianhacker commented Jun 21, 2019

weltenwort left a comment

weltenwort Jun 21, 2019

simianhacker Jun 21, 2019

jasonrhodes Jun 25, 2019

weltenwort Jun 21, 2019

simianhacker Jun 21, 2019

elasticmachine commented Jun 21, 2019

simianhacker commented Jun 21, 2019

elasticmachine commented Jun 21, 2019

skh left a comment

skh Jun 24, 2019

weltenwort Jun 24, 2019

jasonrhodes Jun 24, 2019

weltenwort Jun 24, 2019

jasonrhodes Jun 24, 2019 •

edited

Loading

jasonrhodes Jun 25, 2019

jasonrhodes Jun 25, 2019

elasticmachine commented Jun 25, 2019

elasticmachine commented Jun 25, 2019

skh left a comment

skh Jun 25, 2019

jasonrhodes commented Jun 25, 2019

elasticmachine commented Jun 25, 2019

elasticmachine commented Jul 5, 2019

[Infra UI] Fix Metrics Explorer UI and Server Exceptions #39368

[Infra UI] Fix Metrics Explorer UI and Server Exceptions #39368

Conversation

simianhacker commented Jun 20, 2019

Summary

Checklist

elasticmachine commented Jun 20, 2019

elasticmachine commented Jun 20, 2019

💔 Build Failed

elasticmachine commented Jun 21, 2019

💚 Build Succeeded

weltenwort left a comment

Choose a reason for hiding this comment

elasticmachine commented Jun 21, 2019

💔 Build Failed

simianhacker commented Jun 21, 2019

elasticmachine commented Jun 21, 2019

💔 Build Failed

elasticmachine commented Jun 21, 2019

💔 Build Failed

simianhacker commented Jun 21, 2019

weltenwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 21, 2019

💔 Build Failed

simianhacker commented Jun 21, 2019

elasticmachine commented Jun 21, 2019

💔 Build Failed

skh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonrhodes Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 25, 2019

💚 Build Succeeded

elasticmachine commented Jun 25, 2019

💔 Build Failed

skh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonrhodes commented Jun 25, 2019

elasticmachine commented Jun 25, 2019

💚 Build Succeeded

elasticmachine commented Jul 5, 2019

💚 Build Succeeded

jasonrhodes Jun 24, 2019 •

edited

Loading