Add metrics Feature #7507

jweak · 2023-04-05T13:16:36Z

This PR moves the current metrics implementation to a feature inside packages/core and introduces a new package @k8slens/metrics which exposes injectionTokens for implementing metrics features.

This PR has tokens for following metrics:

Cluster overview metrics
Metrics in resource details (Cluster, Node, Pod, Container, Deployment, StatefulSet, Ingress, VolumeClaim, ReplicaSet, DaemonSet, Job, Namespace)

Metrics from the core can be included with one line of code in open-lens, registerFeature(metricsFeature). Without registering the feature, Lens will not render metrics where those injectionTokens are used.

Resolves #7506

…Feature Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

samitiilikainen · 2023-04-06T11:04:19Z

packages/metrics/package.json

+    "name": "@k8slens/metrics",
+    "private": false,
+    "version": "6.5.0-alpha.1",
+    "description": "Code that is common to all Features and those registering them.",


This should be something about metrics?

good catch, copy-pasta! 😄

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

samitiilikainen · 2023-04-06T11:14:18Z

packages/metrics/tsconfig.json

@@ -0,0 +1,4 @@
+{
+    "extends": "@k8slens/typescript/config/base.json",


Should this be in the dependencies or does it just work?

hmm good question.. no other package imports this but all of them use it... it should probably be a dev dependency for all?

samitiilikainen · 2023-04-06T11:14:39Z

packages/metrics/jest.config.js

@@ -0,0 +1 @@
+module.exports = require("@k8slens/jest").monorepoPackageConfig(__dirname).configForReact;


Should this be in the dependencies or does it just work?

Should be added yeah. Hard to remember because they seem to work anyway because of node module resolution and npm workspaces automatically hoisting dependencies to root of monorepo. If we had something more strict, like pnpm then it would be easier to remember.

PR #7545 makes it bit more strict to remind us.

aleksfront · 2023-04-06T11:19:01Z

...e/src/renderer/components/+workloads-pods/details/containers/pod-details-init-containers.tsx

+  );
+});
+
+const PodDetailsInitContainers = NonInjectedPodDetailsInitContainers;


No need for 2 components since we're not injecting anything here.

good catch, fixed

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

jweak · 2023-04-06T13:35:14Z

Adding it here so I'll remember to fix this. There's a new warning in the console when navigating to Cluster overview:
Cluster Overview, some other page, back to Cluster Overview

Warning: Cannot update a component (`observerComponent`) while rendering a different component (`observerComponent`). To locate the bad setState() call inside `observerComponent`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render
    at observerComponent (https://cf2cda344dbade08f9455be5bd0d5662.lens.app:63776/build/lens.js?03198672fc5ca6362ea6:295016:73)
    at div
    at observerComponent (https://cf2cda344dbade08f9455be5bd0d5662.lens.app:63776/build/lens.js?03198672fc5ca6362ea6:295016:73)
    at div
    at NonInjectedErrorBoundary (https://cf2cda344dbade08f9455be5bd0d5662.lens.app:63776/build/lens.js?03198672fc5ca6362ea6:690197:9)
    at main
    at div
    at observerComponent

jansav · 2023-04-11T04:57:40Z

packages/core/src/renderer/components/+cluster/cluster-overview.tsx

@@ -118,5 +121,6 @@ export const ClusterOverview = withInjectables<Dependencies>(NonInjectedClusterO
    podStore: di.inject(podStoreInjectable),
    eventStore: di.inject(eventStoreInjectable),
    nodeStore: di.inject(nodeStoreInjectable),
+    uiBlocks: di.injectMany(clusterOverviewUIBlockInjectionToken),


Here we should use computedInjectMany to make it reactive. Currently it doesn't refresh if new implementation for clusterOverviewUIBlockInjectionToken is registered in runtime. (or deregistered).

This works at the moment because registration of the Feature is happening before anything is rendered, but it's good practice to always support runtime registrations (less work in the future when we enable possibility to register Features through runtime extensions)

good call, fixed

jansav · 2023-04-11T05:00:50Z

packages/metrics/index.ts

+export type ClusterOverviewUIBlock = {
+  id: string;
+  Component: React.ElementType;
+  orderNumber: number;
+};
+
+export const clusterOverviewUIBlockInjectionToken = getInjectionToken<ClusterOverviewUIBlock>({
+  id: "cluster-overview-ui-block-injection-token",
+});


To me, it seems that these do not belong here. Maybe add a comment to remind that these should be moved under cluster-overview feature whenever it emerges.

added a comment here

jansav · 2023-04-11T05:04:53Z

packages/metrics/index.ts

+export const podDetailsMetricsInjectionToken = getInjectionToken<KubeObjectDetailMetrics>({
+  id: "pod-details-metrics-injection-token",
+});
+
+export const deploymentDetailsMetricsInjectionToken = getInjectionToken<KubeObjectDetailMetrics>({
+  id: "deployment-details-metrics-injection-token",
+});
+
+export const nodeDetailsMetricsInjectionToken = getInjectionToken<KubeObjectDetailMetrics>({
+  id: "node-details-metrics-injection-token",
+});
+
+export const replicaSetDetailsMetricsInjectionToken = getInjectionToken<KubeObjectDetailMetrics>({
+  id: "replica-set-details-metrics-injection-token",
+});
+
+export const persistentVolumeClaimDetailsMetricsInjectionToken = getInjectionToken<KubeObjectDetailMetrics>({
+  id: "persistent-volume-claim-details-metrics-injection-token",
+});
+
+export const statefulSetDetailsMetricsInjectionToken = getInjectionToken<KubeObjectDetailMetrics>({
+  id: "stateful-set-details-metrics-injection-token",
+});
+
+export const namespaceDetailsMetricsInjectionToken = getInjectionToken<KubeObjectDetailMetrics>({


Is it possible to make all of these bit more generic, so that we don't need multiple injectionTokens for same kind of stuff?

I think it's useful to have separate tokens for each resource type. Maybe someone wants to implement metrics just for one resource but not all?

We don't implement metrics for all resource types ourselves. The other thing is that core decides the placement of the metrics (at least for now), for example Pod details have both Pod metrics and Container metrics where Pod metrics are on top of the details and Container lower in the middle of the details component.

jansav · 2023-04-11T05:06:25Z

...object-details/kube-object-detail-items/metrics/getMetricsKubeObjectDetailItem.injectable.ts

+    const metrics = di.injectMany(token);
+    const first = metrics[0];
+
+    const Component = first?.Component ?? EmptyMetrics;


This doesn't seem right. If we expect that there's only one item, then we should use di.inject instead of injectMany, but in this case, I think that the correct solution would be to prepare for the fact that there's multiple implementations and render all of them.

I've changed this to use computedMany

jansav · 2023-04-11T05:13:21Z

...ube-object-details/kube-object-detail-items/metrics/deployment-details-metrics.injectable.ts

@@ -0,0 +1,24 @@
+/**


In general, the guideline should be that the core doesn't know anything about metrics, therefore the fact that metrics exports *MetricsInjectionToken seems bit odd.

It would make more sense if the kubeObjectDetailItemInjectionToken would be extracted to package and then all of these implementations could also be moved.

Might be that you have already thought it, but it cascaded to something too big?

I think it was deemed to be a too big of an effort right now to move the implementations into a package. It was decided that the old implementations would stay in the core because they are too entangled and the benefit of moving is not that big. We want to make these metrics replaceable but not totally remove them from the core at this point.

jansav · 2023-04-11T05:18:03Z

Added some comments. Looking good.

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

jweak · 2023-04-11T08:42:10Z

Adding it here so I'll remember to fix this. There's a new warning in the console when navigating to Cluster overview: Cluster Overview, some other page, back to Cluster Overview

Warning: Cannot update a component (`observerComponent`) while rendering a different component (`observerComponent`). To locate the bad setState() call inside `observerComponent`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render
    at observerComponent (https://cf2cda344dbade08f9455be5bd0d5662.lens.app:63776/build/lens.js?03198672fc5ca6362ea6:295016:73)
    at div
    at observerComponent (https://cf2cda344dbade08f9455be5bd0d5662.lens.app:63776/build/lens.js?03198672fc5ca6362ea6:295016:73)
    at div
    at NonInjectedErrorBoundary (https://cf2cda344dbade08f9455be5bd0d5662.lens.app:63776/build/lens.js?03198672fc5ca6362ea6:690197:9)
    at main
    at div
    at observerComponent

can't reproduce this anymore, maybe fixed by merging master / changing overview ui blocks to computed value

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Iku-turso · 2023-04-12T10:47:41Z

packages/core/src/features/metrics/metrics-feature.ts

@@ -58,5 +73,6 @@ export const metricsFeature = getFeature({

    di.register(podDetailsMetricsInjectable);
    di.register(podDetailsContainerMetricsInjectable);
+    di.register(deploymentDetailsMetricsInjectable);


No action required, this is also possible:

di.register( clusterPieChartsClusterOverviewInjectable, clusterMetricsOverviewBlockInjectable, makeMargaritasInjectable, itPutsTheLotionInTheBasketInjectable );

Iku-turso · 2023-04-12T10:52:32Z

.../kube-object-details/kube-object-detail-items/metrics/pod-details-pod-metrics.injectable.tsx

  instantiate: (di) => {
    const podMetrics = di.injectMany(podDetailsMetricsInjectionToken);
    const first = podMetrics[0];

-    const Component = first ?? Empty;
+    const Component = first?.Component ?? EmptyMetrics;


Could we instead register EmptyMetrics as implementation of podDetailsMetricsInjectionToken when there's nothing better around?

Iku-turso · 2023-04-12T11:19:15Z

...bject-details/kube-object-detail-items/metrics/getMetricsKubeObjectDetailItem.injectable.tsx

+
+type GetMetricsKubeObjectDetailItem = (token: InjectionToken<KubeObjectDetailMetrics, void>, metricResourceType: ClusterMetricsResourceType) => KubeObjectDetailItem;
+
+export const getMetricsKubeObjectDetailItemInjectable = getInjectable({


Does this have to be a factory function? Eg. instead of getMetricsKubeObjectDetailItemInjectable, could it be metricsKubeObjectDetailItemInjectable, and receive metricResourceType as its key as keyedSingleton?

Iku-turso · 2023-04-12T11:30:10Z

packages/core/src/renderer/components/+cluster/cluster-overview.tsx

@@ -121,6 +122,6 @@ export const ClusterOverview = withInjectables<Dependencies>(NonInjectedClusterO
    podStore: di.inject(podStoreInjectable),
    eventStore: di.inject(eventStoreInjectable),
    nodeStore: di.inject(nodeStoreInjectable),
-    uiBlocks: di.injectMany(clusterOverviewUIBlockInjectionToken),
+    uiBlocks: di.inject(computedInjectManyInjectable)(clusterOverviewUIBlockInjectionToken),


Shout if you want di.computedInjectMany(...)...

That'd be useful!

jweak added 15 commits April 5, 2023 15:59

Add cluster UI block injection token and current implementation as a …

66f3cc8

…Feature Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Add pod details metrics injection token.

be0028c

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Extract pod container metrics to its own injection token.

03a87f9

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Add deployment details metrics injection token

4401b17

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Add node details metrics injection token.

fdff10a

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Add replicaset details metrics injection token.

8adebb6

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Add persistent volume claim details metrics injection token.

ecd8af3

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Refactor repeating code into a separate function.

21df918

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Add stateful set details metrics injection token.

275a9f7

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Add namespace details metrics injection token.

669e641

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Rename details injectables file suffixes from tsx to ts

d439252

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Add job details metrics injection token.

0339870

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Add daemonset details metrics injection token.

e725e41

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Add ingress details metrics injection token.

d13c5fa

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Remove duplicate package import

7e06047

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

jweak requested review from panuhorsmalahti, jansav, Iku-turso and samitiilikainen April 5, 2023 13:16

jweak requested a review from a team as a code owner April 5, 2023 13:16

Update package-lock.json

afd64df

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

jweak force-pushed the metrics-extraction branch from 9142744 to afd64df Compare April 5, 2023 13:17

jweak added enhancement New feature or request area/metrics All the things related to metrics labels Apr 5, 2023

jweak added this to the 6.5.0 milestone Apr 5, 2023

samitiilikainen reviewed Apr 6, 2023

View reviewed changes

Update metrics package description

8e4774e

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

samitiilikainen reviewed Apr 6, 2023

View reviewed changes

aleksfront reviewed Apr 6, 2023

View reviewed changes

jweak added 4 commits April 6, 2023 14:39

Remove unnecessary variable from init containers component

07b4e30

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Add missing packages to metrics package.json

a8fd71e

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Merge branch 'master' into metrics-extraction

e0040e1

Lint fixes after changed prettier config

9ff084a

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

jansav reviewed Apr 11, 2023

View reviewed changes

jweak added 3 commits April 11, 2023 10:48

Update package-lock.json

fc463b7

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Make Cluster overview UI blocks computed value

1631184

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Merge branch 'master' into metrics-extraction

a1b2f08

jweak added 4 commits April 11, 2023 11:45

Add comment for moving ui blocks to own package

f858637

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Make details metrics a computed value.

0e0b54b

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

Merge branch 'master' into metrics-extraction

0101d6a

Merge branch 'master' into metrics-extraction

5e6e6c0

jweak requested a review from jansav April 12, 2023 09:07

Iku-turso reviewed Apr 12, 2023

View reviewed changes

Iku-turso approved these changes Apr 12, 2023

View reviewed changes

jweak merged commit 53cb3a5 into master Apr 12, 2023

jweak deleted the metrics-extraction branch April 12, 2023 11:40

Nokel81 mentioned this pull request Apr 12, 2023

Release 6.5.0-alpha.4 #7535

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metrics Feature #7507

Add metrics Feature #7507

jweak commented Apr 5, 2023

samitiilikainen Apr 6, 2023

jweak Apr 6, 2023

jweak Apr 6, 2023

samitiilikainen Apr 6, 2023

jweak Apr 6, 2023

jweak Apr 6, 2023

samitiilikainen Apr 6, 2023

jweak Apr 6, 2023

jansav Apr 11, 2023 •

edited

Loading

jansav Apr 13, 2023 •

edited

Loading

aleksfront Apr 6, 2023

jweak Apr 6, 2023

jweak commented Apr 6, 2023 •

edited

Loading

jansav Apr 11, 2023

jweak Apr 11, 2023

jansav Apr 11, 2023

jweak Apr 11, 2023

jansav Apr 11, 2023

jweak Apr 11, 2023

jansav Apr 11, 2023

jweak Apr 11, 2023

jansav Apr 11, 2023 •

edited

Loading

jweak Apr 11, 2023

jansav commented Apr 11, 2023 •

edited

Loading

jweak commented Apr 11, 2023

Iku-turso Apr 12, 2023 •

edited

Loading

Iku-turso Apr 12, 2023

Iku-turso Apr 12, 2023

Iku-turso Apr 12, 2023

jweak Apr 12, 2023

		@@ -0,0 +1,4 @@
		{
		"extends": "@k8slens/typescript/config/base.json",

		@@ -0,0 +1 @@
		module.exports = require("@k8slens/jest").monorepoPackageConfig(__dirname).configForReact;


		type GetMetricsKubeObjectDetailItem = (token: InjectionToken<KubeObjectDetailMetrics, void>, metricResourceType: ClusterMetricsResourceType) => KubeObjectDetailItem;

		export const getMetricsKubeObjectDetailItemInjectable = getInjectable({

Add metrics Feature #7507

Add metrics Feature #7507

Conversation

jweak commented Apr 5, 2023

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

jansav Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

jansav Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jweak commented Apr 6, 2023 • 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

jansav Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jansav commented Apr 11, 2023 • edited Loading

jweak commented Apr 11, 2023

Iku-turso Apr 12, 2023 • 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

jansav Apr 11, 2023 •

edited

Loading

jansav Apr 13, 2023 •

edited

Loading

jweak commented Apr 6, 2023 •

edited

Loading

jansav Apr 11, 2023 •

edited

Loading

jansav commented Apr 11, 2023 •

edited

Loading

Iku-turso Apr 12, 2023 •

edited

Loading