[Infra Monitoring UI] Add event module filter to metrics table #133872

crespocarlos · 2022-06-08T11:11:40Z

Summary

this PR fixes #131308 by adding event.module filter to metrics table queries. For hosts it will filter by system and for pods and containers by kubernetes. This will filter out some undesired rows without relevant metric data.

Example

Without the filter:

With filter:

Checklist

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

crespocarlos · 2022-06-08T12:57:12Z

@elasticmachine merge upstream

crespocarlos · 2022-06-08T15:02:23Z

@elasticmachine merge upstream

elasticmachine · 2022-06-09T07:04:42Z

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

miltonhultgren

Nice! Thanks for adding the tests as well!

I would still want @weltenwort to approve as well on the DSL query we end up with, so we don't risk breaking future usages.

miltonhultgren · 2022-06-09T07:44:27Z

...ublic/components/infrastructure_node_metrics_tables/container/use_container_metrics_table.ts

@@ -81,6 +87,25 @@ export function useContainerMetricsTable({
  };
 }

+function addEventModuleFilter(


Suggested change

function addEventModuleFilter(

function addKubernetesModuleFilter(

I feel this would be more clear about which kind of event we want to filter on?

crespocarlos · 2022-06-13T09:16:33Z

@elasticmachine merge upstream

weltenwort

Thanks indeed for adding the hook unit tests.

weltenwort · 2022-06-14T11:00:59Z

...ublic/components/infrastructure_node_metrics_tables/container/use_container_metrics_table.ts

+        {
+          ...filterClauseDsl,
+        },


When typeof filterClauseDsl === 'undefined', then this would put an empty object into the filter array. How about we avoid adding it in the first place?

Suggested change

{

...filterClauseDsl,

},

...(filterClauseDsl != null ? [filterClauseDsl] : []),

good catch! I'll just change your suggestion for an array to an object

but we're merging into an array? 🤔

ah, I see now. At first glance I thought you were adding it to the filter array. You are actually concatenating. Got it

...ublic/components/infrastructure_node_metrics_tables/container/use_container_metrics_table.ts

weltenwort · 2022-06-14T11:25:43Z

...ns/infra/public/components/infrastructure_node_metrics_tables/host/use_host_metrics_table.ts

+      filter: [
+        {
+          term: {
+            'event.module': 'system',


This is closely coupled to the knowledge that's encoded in hostMetricsMap above. Would there be a reasonable way to automatically derive this from the map? Or at least co-locate this magic string so it doesn't diverge?

What if, instead of filtering for the module, we filtered for the presence of the metrics fields from hostMetricsMap directly? It would me more parsimonious than introducing another constant.

I agree with your concern, and we thought about this but the risk is that other documents populate the same fields and are they then the "same" value or not? See #131308 (comment)

Valid point. But the metric field names are rather specific, so I don't imagine the likelihood would be very high. Or do you have a specific confounding source in mind? Additionally, the confounding source could also erroneously set the module name.

In the ticket I linked, Carlos has data from the APM agent that has the same fields (not all, but some) for the Host table (not sure if the values match what the system module would report though) but in this case it doesn't report the module "wrong". So I don't know :D I don't have a good answer here

hm, yes... for now the module filter seems like the simplest workaround. still, could we co-locate that knowledge somehow with the metric field names, @crespocarlos?

sure! I'll see how that can be improved.

The refactoring was a slightly bigger than I expected, but I think now the knowledge about how the query will need is centralized

crespocarlos · 2022-06-15T14:12:47Z

@elasticmachine merge upstream

crespocarlos · 2022-06-15T15:16:21Z

buildkite test this

crespocarlos · 2022-06-15T16:27:39Z

@elasticmachine merge upstream

weltenwort

Thanks for combining the metrics descriptors 👏 I left a few more questions below.

weltenwort · 2022-06-15T15:08:20Z

...ublic/components/infrastructure_node_metrics_tables/container/use_container_metrics_table.ts

+  const [containerMetricsOptions, setContainerMetricsOptions] = useState<MetricsExplorerOptions>();
+
+  useEffect(() => {
+    if (!containerMetricsOptions) {
+      const { options } = metricsToApiOptions(containerMetricsQueryConfig, filterClauseDsl);
+      setContainerMetricsOptions(options);
+    }
+  }, [filterClauseDsl, containerMetricsOptions]);
+


Does this have to be mutable state? Or could this be computed state (e.g. with useMemo)?

hmm, indeed. I think when I first decided to go with useEffect I had dependencies that were mutable. Despite filterClauseDsl being passed via props, it is predefined on the client and never changes. I guess we can go with useMemo here. There is no reason to delay the first render here.

.../public/components/infrastructure_node_metrics_tables/shared/hooks/metrics_to_api_options.ts

weltenwort · 2022-06-15T16:13:33Z

...ic/components/infrastructure_node_metrics_tables/shared/hooks/metrics_to_api_options.test.ts

    expect(options).toEqual({
      aggregation: 'avg',
      groupBy: 'test.node.type.groupingField',
+      filterQuery: JSON.stringify({ bool: { filter: [{ term: { 'event.module': 'test' } }, {}] } }),


We don't want this empty object in here, right?

yup. it's outdated

weltenwort · 2022-06-15T16:14:27Z

...ic/components/infrastructure_node_metrics_tables/shared/hooks/metrics_to_api_options.test.ts

@@ -5,44 +5,59 @@
 * 2.0.
 */

-import type { MetricsMap } from './metrics_to_api_options';
+import type { MetricsExplorerOptions } from '../../../../pages/metrics/metrics_explorer/hooks/use_metrics_explorer_options';
+import { createMetricByFieldLookup, MetricsQueryOptions } from './metrics_to_api_options';


Do we want to let the compiler auto-sort and deduplicate these imports (and others in the PR)?

I was too optmistic that vscode would've added the new import to the existing list

weltenwort · 2022-06-15T16:29:36Z

.../public/components/infrastructure_node_metrics_tables/shared/hooks/metrics_to_api_options.ts

+  const fields = Object.keys(metricMap) as T[];
+  const metrics = Object.values(metricMap) as Array<NodeMetricsExplorerOptionsMetric<T>>;


Nit: These assertions could be even safer if we let the compiler derive the types, for example:

Suggested change

const fields = Object.keys(metricMap) as T[];

const metrics = Object.values(metricMap) as Array<NodeMetricsExplorerOptionsMetric<T>>;

const fields = Object.keys(metricMap) as Array<keyof typeof metricMap>;

const metrics = Object.values(metricMap) as ObjectValues<typeof metricMap>;

where ObjectValues<T> is defined in

kibana/x-pack/plugins/infra/common/utility_types.ts

Line 47 in 1c08fb4

export type ObjectValues<T> = Array<T[keyof T]>;

What do you think?

nice! seems neat

weltenwort · 2022-06-15T16:30:49Z

...infra/public/components/infrastructure_node_metrics_tables/pod/use_pod_metrics_table.test.ts

+
+    const filterClauseWithEventModuleFilter = {
+      bool: {
+        filter: [{ term: { 'event.module': 'kubernetes' } }, { ...filterClauseDsl }],


This expected value might be outdated.

it sure is!

weltenwort · 2022-06-15T16:35:24Z

...ublic/components/infrastructure_node_metrics_tables/container/use_container_metrics_table.ts

-  'kubernetes.container.memory.usage.bytes': {
-    aggregation: 'avg',
-    field: 'kubernetes.container.memory.usage.bytes',
+  groupByField: 'kubernetes.pod.name',


Could this incorrectly mix the metrics of multiple containers since a pod might have multiple containers? Would kubernetes.container.id be more suitable?

Good catch. It should indeed be container.id.

miltonhultgren

LGTM 👍🏼

miltonhultgren · 2022-06-15T17:43:42Z

.../public/components/infrastructure_node_metrics_tables/shared/hooks/metrics_to_api_options.ts

+  return {
+    bool: {
+      filter: !!filterClauseDsl ? [sourceFilter, filterClauseDsl] : [sourceFilter],
+    },
+  };


Suggested change

return {

bool: {

filter: !!filterClauseDsl ? [sourceFilter, filterClauseDsl] : [sourceFilter],

},

};

const filter = [sourceFilter];

if (filterClauseDsl) {

filter.push(filterClauseDsl);

}

return {

bool: {

filter,

},

};

NIt: I'm just allergic to ternaries for the sake of readability.

I tend to agree with that. But here it doesn't seem to be harming the readability. But it's my personal opinion. I could apply your suggestion if it's hurting you eyes :)!

One could argue that mutating the list can be considered an eyesore too 😉

The mutation hurts the soul, not the eyes :D
I'm fine either way @crespocarlos, as long as I get to complain about ternaries :)

miltonhultgren · 2022-06-15T17:50:26Z

...gins/infra/public/components/infrastructure_node_metrics_tables/pod/use_pod_metrics_table.ts

 import { metricsToApiOptions, useInfrastructureNodeMetrics } from '../shared';
+import { createMetricByFieldLookup } from '../shared/hooks/metrics_to_api_options';


Nit: Maybe export/import this via ../shared instead?

crespocarlos · 2022-06-15T20:37:30Z

buildkite test this

weltenwort

LGTM, nicely done

weltenwort · 2022-06-16T10:11:51Z

...ns/infra/public/components/infrastructure_node_metrics_tables/host/use_host_metrics_table.ts

 import { metricsToApiOptions, useInfrastructureNodeMetrics } from '../shared';
+import { createMetricByFieldLookup } from '../shared/hooks/metrics_to_api_options';


tiny nit: looks like this could be imported from ../shared like in the sibling hook variants 😇

…le-filter-to-metrics-table

crespocarlos · 2022-06-16T13:17:47Z

buildkite test this

kibana-ci · 2022-06-16T14:18:57Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`infra`	1001.4KB	1002.1KB	+726.0B

History

💛 Build #51325 was flaky da0ca31
💛 Build #51305 was flaky 1bbdae0
💚 Build #51235 succeeded 6e30166
💛 Build #51192 was flaky 6e30166
💔 Build #51148 failed 568ba85
💔 Build #51114 failed 620a2cd

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

add event module filter to metrics table

116bea2

crespocarlos added release_note:fix Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services backport:skip This commit does not require backporting v8.4.0 labels Jun 8, 2022

crespocarlos changed the title ~~add event module filter to metrics table~~ [Metrics UI] add event module filter to metrics table Jun 8, 2022

crespocarlos changed the title ~~[Metrics UI] add event module filter to metrics table~~ [Metrics UI] Add event module filter to metrics table Jun 8, 2022

crespocarlos added 2 commits June 8, 2022 13:36

license clean up

8dee579

typing clean up

47298ca

Merge branch 'main' into 131308-add-module-filter-to-metrics-table

e744e0f

Merge branch 'main' into 131308-add-module-filter-to-metrics-table

9663f86

crespocarlos marked this pull request as ready for review June 9, 2022 07:04

crespocarlos requested a review from a team as a code owner June 9, 2022 07:04

miltonhultgren approved these changes Jun 9, 2022

View reviewed changes

Merge branch 'main' into 131308-add-module-filter-to-metrics-table

7ed4669

weltenwort reviewed Jun 14, 2022

View reviewed changes

crespocarlos added 2 commits June 15, 2022 15:53

refactor how metrics api options is built

7d7868d

small fix on how the filter clause is built

5c4b324

Merge branch 'main' into 131308-add-module-filter-to-metrics-table

620a2cd

crespocarlos changed the title ~~[Metrics UI] Add event module filter to metrics table~~ [Infra Monitoring UI] Add event module filter to metrics table Jun 15, 2022

Merge branch 'main' into 131308-add-module-filter-to-metrics-table

568ba85

weltenwort reviewed Jun 15, 2022

View reviewed changes

crespocarlos added 2 commits June 15, 2022 19:19

fix unit tests, container group by field name and small improvements

46a6da7

remove optional from metricsExplorerOptions parameter

4047ce0

miltonhultgren approved these changes Jun 15, 2022

View reviewed changes

fix the export of createMetricByFieldLookup function

6e30166

weltenwort approved these changes Jun 16, 2022

View reviewed changes

crespocarlos added 2 commits June 16, 2022 12:24

fix import of createMetricByFieldLookup function

1bbdae0

Merge branch 'main' of github.com:elastic/kibana into 131308-add-modu…

da0ca31

…le-filter-to-metrics-table

crespocarlos merged commit b401a7d into elastic:main Jun 16, 2022

crespocarlos deleted the 131308-add-module-filter-to-metrics-table branch June 16, 2022 14:20

	function addEventModuleFilter(
	function addKubernetesModuleFilter(

		const fields = Object.keys(metricMap) as T[];
		const metrics = Object.values(metricMap) as Array<NodeMetricsExplorerOptionsMetric<T>>;

		import { metricsToApiOptions, useInfrastructureNodeMetrics } from '../shared';
		import { createMetricByFieldLookup } from '../shared/hooks/metrics_to_api_options';

[Infra Monitoring UI] Add event module filter to metrics table #133872

[Infra Monitoring UI] Add event module filter to metrics table #133872

Conversation

crespocarlos commented Jun 8, 2022

Summary

Example

Checklist

For maintainers

crespocarlos commented Jun 8, 2022

crespocarlos commented Jun 8, 2022

elasticmachine commented Jun 9, 2022

miltonhultgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos commented Jun 13, 2022

weltenwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos Jun 14, 2022 • edited Loading

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

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

crespocarlos commented Jun 15, 2022

crespocarlos commented Jun 15, 2022

crespocarlos commented Jun 15, 2022

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

weltenwort Jun 15, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miltonhultgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos Jun 15, 2022 • edited Loading

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

crespocarlos commented Jun 15, 2022

weltenwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos commented Jun 16, 2022

kibana-ci commented Jun 16, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

History

crespocarlos Jun 14, 2022 •

edited

Loading

weltenwort Jun 15, 2022 •

edited

Loading

crespocarlos Jun 15, 2022 •

edited

Loading