Allow multiple metrics per chart #87

jotak · 2020-04-09T15:44:32Z

Changed MonitoringDashboard Spec: new "Metrics" field (list of metrics within a chart)
o Backward-compatible from user point of view (no need to update dashboards)...
o But breaking change from developer point of view, consuming side (Kiali) needs to adapt
Updated fetching process
Updated model on front/back interface: removed distinction "metric vs histogram", now there's several metrics and histograms are just a (not-so-)special case of that
Updated tests, both front and back; updated front stories

Depends on kiali/k-charted#87 Fixes kiali/kiali#1778

Depends on kiali/k-charted#87 Fixes kiali#1778

- Changed MonitoringDashboard Spec: new "Metrics" field (list of metrics within a chart) o Backward-compatible from user point of view (no need to update dashboards)... o But breaking change from developer point of view, consuming side (Kiali) needs to adapt - Updated fetching process - Updated model on front/back interface: removed distinction "metric vs histogram", now there's several metrics and histograms are just a special case of that - Updated tests, both front and back; updated front stories

Depends on kiali/k-charted#87 Fixes kiali#1778

israel-hdez

Partial review, yet. Just a single question for now.

israel-hdez · 2020-04-20T22:16:59Z

web/common/utils/timeSeriesUtils.ts

+    const name = ts.labelSet[nameLabel];
+    const stat = mapStatForDisplay(ts.labelSet[statLabel]);
+    const otherLabels = Object.keys(ts.labelSet)
+      .filter(k => k !== nameLabel && k !== statLabel)


Filtering out the reporter label is removed. Is it no longer necessary?
In a quick scan to the go-changes is unclear for me.

perhaps a mistake, let me check in kiali ...

Actually, this was an old filter that has turned useless a while ago, it can be safely removed. At that time, we used to fetch metrics for both reporters (grouped by reporter) that why it was appearing in labels and had to be filtered out. Today, metrics are fetched filtered by reporter, so that it doesn't appear in labels.

And anyway, even if we had to do something about reporters, it should be in kiali, not in k-charted, because k-charted doesn't assume metrics are the istio ones.

Depends on kiali/k-charted#87 Fixes kiali#1778

Depends on kiali/k-charted#87 Fixes kiali/kiali#1778

jotak · 2020-04-23T08:27:13Z

For reviewers, if you want to check storybook, the one that correspond to this change is the "PF4 Dashboard" / "Several metrics ..." :

lucasponce · 2020-04-23T13:55:28Z

kubernetes/v1alpha1/spec.go

@@ -58,11 +58,17 @@ type MonitoringDashboardChart struct {
 	Min          *int                             `json:"min"`
 	Max          *int                             `json:"max"`
 	MetricName   string                           `json:"metricName"`
+	Metrics      []MonitoringDashboardMetric      `json:"metrics"`


Now that this library it's part of production versions like ServiceMesh 1.0 and ServiceMesh 1.1.
Have we thought if these changes may have some impact in the operator work when upgrading things ?

I'm inclined to think that not, it's fine.
A SM 1.1 that upgrades to SM 1.2 will reset it's specification and that's fine.

But I just leave this question here in case there are some other aspect that I missed.

In case of yes, perhaps it would need some process to upgrade one version to another.

In general I haven't seen a strong use of v1alpha1 -> v1alpha2 etc, I don't want to handle this versioning for this.
It's overarch, but I think that review about if operator should be able to update the CRDs from SM 1.1 to SM 1.2 (which may include this) and what should do with existing CRs (perhaps a patch).

I don't expect an action on the PR about this, but perhaps an Issue to not forget and describing any additional action on that side.

Same as you, I don't see any reason why it wouldn't work, I hope I'm not missing something. The fact that we don't change the CRD should precisely make the things easier / without problem, I think.

We had a related discussion in the past when trying to incorporate the schemas to the CRDs. We found that the "de-facto" is to not change CRD version if you are doing changes in a backwards compatible way. CRD version is updated only when a breaking change occurs.

So, I'm +1 with keeping v1alpha1.

If, at some point you need to "bump" the CRD version, since this is integrated into Kiali, there are some guidelines to follow because an "operator" and OLM are involved. I couldn't find the page in a quick search, but the idea is that you cannot simply drop support for the old CRD version. It's a two step process. You need to first release a transitional version of the operator supporting both the old and the new versions of the CRD, in a subsequent release of the operator, you can then drop support for the old one and keep only the new one. In the transitional release, you may need to provide a conversion webhook to make the "automatic" upgrade of the old CRs and clear the way for dropping support of the old CRD. An user using OLM, will forcedly pass through the transitional version of the operator and may not realize that CRDs were upgraded and deprecated (specially if they use automatic upgrades). It's not that easy to bump CRD versions :/

lucasponce · 2020-04-23T14:01:21Z

kubernetes/v1alpha1/spec.go

@@ -58,11 +58,17 @@ type MonitoringDashboardChart struct {
 	Min          *int                             `json:"min"`
 	Max          *int                             `json:"max"`
 	MetricName   string                           `json:"metricName"`
+	Metrics      []MonitoringDashboardMetric      `json:"metrics"`


MonitoringDashboardChart has a field called metricName (in json terminology).
Now it has also an array field as "metrics" which is a list of metricsName/displayName.

From external point of view it makes model less clear.
My first understanding is there is a main metric and the other are optional/subordinated.

I'm not sure if I'm understanding correctly what it represents. If MonitoringDashboardChart is going to support 1:N metrics I'd expect to see them all in a single place (and having an array of length == 1 for single case, I think).

This is correct to say that there's all in a single place and metrics array of size 1 does represent the old case. This is actually what happens in the code.

But metricName is kept for backward compatibility, and its content is used to fill the metrics array. I will explain that in the documentation (in kiali.io), and mark that as deprecated. This is a transitional state

I've added a comment to mark metricName Deprecated

lucasponce · 2020-04-23T14:15:30Z

kubernetes/mock/mock.go

+		Unit:      "s",
+		UnitScale: 10.0,
+		Spans:     6,
+		Metrics:   []v1alpha1.MonitoringDashboardMetric{{DisplayName: "My chart " + id, MetricName: "my_metric_" + id}},


Here I see the "MetricName" is not used on MonitoringDashboardChart.
Is that intended ? I mean, perhaps it was left intentional for some compability ?

That trigger is on this case we would need a v1alpha2 or v1beta1 then, not sure, I don't want to enter in that versioning but I'm 50/50 seeing changes in the model on that sense.

I've just updated the mock to use the new model and not MetricName which I deprecate.
I don't think it's worth it to bump versioning, as long as we keep backward compatibility, old dashboards continue to work and we don't have to go into the trouble of versioning right now.

lucasponce · 2020-04-23T14:17:37Z

model/dashboards.go

+	ChartType *string         `json:"chartType,omitempty"`
+	Min       *int            `json:"min,omitempty"`
+	Max       *int            `json:"max,omitempty"`
+	Metrics   []*SampleStream `json:"metrics"`


Here it seems updated correctly, so all metrics are under the Metrics array as expected.

No, I'm wrong, these are the metrics raw data.

lucasponce · 2020-04-23T14:46:49Z

web/common/utils/timeSeriesUtils.ts

+    hasSeveralFamilyNames = series.some(s => s.labelSet[nameLabel] !== firstName);
+  }
+  return series.map(ts => {
+    const name = ts.labelSet[nameLabel];


If global constants are in capital letters it could be easy to check if there is a missing variable, nameLabel and statLabel perhaps can go to NAME_LABEL, STAT_LABEL for better readability.
I think.

The thing is that, it's true that in kiali-ui we have no consensus over how to write constants, we deliberately allow the two forms in our style guide: https://github.com/kiali/kiali-ui/blob/master/STYLE_GUIDE.adoc#variables-functions-constants.

In k-charted, at the moment it's still strictly following Basarat's guide (so a stricter version of our guide in kiali-ui), which makes no distinction between exported/not exported constants/variables (all camel case). There's other global constants in k-charted that are camel case, and none that are upper case. So I would prefer to remain consistent, and if we think that's a problem, then change at every place.

lucasponce · 2020-04-23T15:24:13Z

I wonder if in terms of visualization:

The "Single metric with label" could be similar as format used in "Several metrics with label",

I mean, do we want to put the metric name too ? instead of single "green, big" to bring the "appels [green, big]"

I know it's minor but at the moment you introduce multiple metrics in a chart you would expect that.

israel-hdez

Hi, Joel

I couldn't finish a "deep" review. I can understand what's happening in the front-end side, but I think I have lost track of the back-end side because I can understand only partially.

I'll try to use some weekend time to re-study k-charted so that I can be more useful here and do better reviews. I don't want to block, so I'll approve to let you merge (unless you want to wait for me).

FWIW, I like that the back-end is now hiding Prometheus Histograms to the front-end side. The front-end is now acting more closely to a standard charting library that just needs to understand series :)

israel-hdez · 2020-04-23T19:51:29Z

kubernetes/v1alpha1/spec.go

@@ -82,6 +88,18 @@ type MonitoringDashboardExternalLinkVariables struct {
 	Workload  string `json:"workload,omitempty"`
 }

+func (in *MonitoringDashboardChart) GetMetrics() []MonitoringDashboardMetric {


So, this is what gives backwards compatibility.
I'd give priority to the non-deprecated field: return Metrics if it is non-empty. Otherwise, build MonitoringDashboardMetric from the deprecated MetricName field. This way, if both are filled, you return the preferred one.

It's my preference. No action needed if you think it's better as is.

yes it makes sense

done. I've also added some code comments here and there

israel-hdez · 2020-04-23T20:19:29Z

kubernetes/v1alpha1/spec.go

@@ -58,11 +58,17 @@ type MonitoringDashboardChart struct {
 	Min          *int                             `json:"min"`
 	Max          *int                             `json:"max"`
 	MetricName   string                           `json:"metricName"`
+	Metrics      []MonitoringDashboardMetric      `json:"metrics"`


We had a related discussion in the past when trying to incorporate the schemas to the CRDs. We found that the "de-facto" is to not change CRD version if you are doing changes in a backwards compatible way. CRD version is updated only when a breaking change occurs.

So, I'm +1 with keeping v1alpha1.

If, at some point you need to "bump" the CRD version, since this is integrated into Kiali, there are some guidelines to follow because an "operator" and OLM are involved. I couldn't find the page in a quick search, but the idea is that you cannot simply drop support for the old CRD version. It's a two step process. You need to first release a transitional version of the operator supporting both the old and the new versions of the CRD, in a subsequent release of the operator, you can then drop support for the old one and keep only the new one. In the transitional release, you may need to provide a conversion webhook to make the "automatic" upgrade of the old CRs and clear the way for dropping support of the old CRD. An user using OLM, will forcedly pass through the transitional version of the operator and may not realize that CRDs were upgraded and deprecated (specially if they use automatic upgrades). It's not that easy to bump CRD versions :/

jotak · 2020-04-24T07:49:35Z

I wonder if in terms of visualization:
(image)
The "Single metric with label" could be similar as format used in "Several metrics with label",

I mean, do we want to put the metric name too ? instead of single "green, big" to bring the "appels [green, big]"

I know it's minor but at the moment you introduce multiple metrics in a chart you would expect that.

My rationale is to save character space in the legend whenever possible, because they are not cheap :-s

When we show metrics per X, Y and Z labels, the number of timeseries grows a lot, and so does the legend, which at some point can become bigger than the chart itself, so that's why I came up removing repeating text as it can be implicitly guessed. Here, it can be guessed because when there's a single serie defined in chart, we can assume the chart title gives the information about what is being displayed.

And this is actually how it currently works in Kiali, look for instance at the request count metric: it displays the name, but if you activate some grouping by label, then the series name disappears to show only labels.

Now, I agree to say this is arguable especially now that the series name can be different from the chart title. So it comes down to having good chart definitions in CRs.

jotak · 2020-04-24T07:58:27Z

I'm going to add some code comments

jotak · 2020-04-27T06:39:16Z

@lucasponce I think I answered all your comments, or is there any remaining problem that prevents merging?

aljesusg

LGFM, tested it

lucasponce

LGTM

Thanks. Main concern was about versioning, rest of my comments were more clarifications to understand better the scenario.

jotak · 2020-04-27T09:48:26Z

Thanks for the reviews

Depends on kiali/k-charted#87 Fixes kiali/kiali#1778

Depends on kiali/k-charted#87 Fixes kiali#1778

Update swagger Depends on kiali/k-charted#87 Fixes kiali#1778

* Allow multiple metrics per chart (model update) Update swagger Depends on kiali/k-charted#87 Fixes #1778 * Catch errors while fetching istio metrics: more consistent charts displaying This change will allow to always display the relevant istio charts, even when it has no data or errors Also moving the duration check (seconds/millis, for old istio compatibility) into a separate function for clarity

* Allow multiple metrics per chart Depends on kiali/k-charted#87 Fixes kiali/kiali#1778 * Fix failing tests

jotak added a commit to jotak/swsui that referenced this pull request Apr 9, 2020

Allow multiple metrics per chart

d0cee95

Depends on kiali/k-charted#87 Fixes kiali/kiali#1778

jotak mentioned this pull request Apr 9, 2020

Allow multiple metrics per chart kiali/kiali-ui#1730

Merged

jotak added a commit to jotak/swscore that referenced this pull request Apr 9, 2020

Allow multiple metrics per chart (model update)

e5a54a2

Depends on kiali/k-charted#87 Fixes kiali#1778

jotak mentioned this pull request Apr 9, 2020

Allow multiple metrics per chart (model update) kiali/kiali#2643

Merged

jotak added a commit to jotak/istio-federated-prometheus that referenced this pull request Apr 9, 2020

Update Kiali dashboard (needs kiali/k-charted#87)

99be9ca

jotak force-pushed the dashboard-multiple-metrics branch from 8e9c3f4 to 5324626 Compare April 10, 2020 13:20

jotak marked this pull request as ready for review April 10, 2020 13:29

jotak added a commit to jotak/swscore that referenced this pull request Apr 10, 2020

Allow multiple metrics per chart (model update)

62db6bc

Depends on kiali/k-charted#87 Fixes kiali#1778

jotak force-pushed the dashboard-multiple-metrics branch from 5324626 to 51dc5f3 Compare April 10, 2020 13:49

jotak added a commit to jotak/swscore that referenced this pull request Apr 10, 2020

Allow multiple metrics per chart (model update)

964d562

Depends on kiali/k-charted#87 Fixes kiali#1778

jotak mentioned this pull request Apr 10, 2020

Test Kiali with best-practice prometheus guide kiali/kiali#2518

Closed

1 task

jotak requested review from aljesusg and israel-hdez April 20, 2020 08:28

israel-hdez reviewed Apr 20, 2020

View reviewed changes

israel-hdez self-requested a review April 20, 2020 22:17

jotak added a commit to jotak/swscore that referenced this pull request Apr 21, 2020

Allow multiple metrics per chart (model update)

6e5fc24

Depends on kiali/k-charted#87 Fixes kiali#1778

jotak added a commit to jotak/swsui that referenced this pull request Apr 21, 2020

Allow multiple metrics per chart

56f869d

Depends on kiali/k-charted#87 Fixes kiali/kiali#1778

jotak assigned aljesusg Apr 22, 2020

jotak requested a review from lucasponce April 23, 2020 08:10

lucasponce reviewed Apr 23, 2020

View reviewed changes

Mark metricName as deprecated in Spec

4ac2ea3

israel-hdez approved these changes Apr 24, 2020

View reviewed changes

Prioritize non-deprecated Metrics field, add code comments, renamed func

bd8d572

aljesusg approved these changes Apr 27, 2020

View reviewed changes

lucasponce approved these changes Apr 27, 2020

View reviewed changes

jotak merged commit 5ae8b0f into kiali:master Apr 27, 2020

jotak added a commit to jotak/swsui that referenced this pull request Apr 27, 2020

Allow multiple metrics per chart

97ea541

Depends on kiali/k-charted#87 Fixes kiali/kiali#1778

jotak added a commit to jotak/swscore that referenced this pull request Apr 27, 2020

Allow multiple metrics per chart (model update)

e79efbb

Depends on kiali/k-charted#87 Fixes kiali#1778

jotak added a commit to jotak/swscore that referenced this pull request Apr 27, 2020

Allow multiple metrics per chart (model update)

c92e90f

Depends on kiali/k-charted#87 Fixes kiali#1778

jotak added a commit to jotak/swscore that referenced this pull request Apr 27, 2020

Allow multiple metrics per chart (model update)

173dfd5

Update swagger Depends on kiali/k-charted#87 Fixes kiali#1778

lucasponce pushed a commit to kiali/kiali-ui that referenced this pull request Apr 30, 2020

Allow multiple metrics per chart (#1730)

50f8197

* Allow multiple metrics per chart Depends on kiali/k-charted#87 Fixes kiali/kiali#1778 * Fix failing tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple metrics per chart #87

Allow multiple metrics per chart #87

jotak commented Apr 9, 2020 •

edited

Loading

israel-hdez left a comment

israel-hdez Apr 20, 2020

jotak Apr 21, 2020

jotak Apr 21, 2020

jotak commented Apr 23, 2020

lucasponce Apr 23, 2020

jotak Apr 23, 2020

israel-hdez Apr 23, 2020

lucasponce Apr 23, 2020 •

edited

Loading

jotak Apr 23, 2020

jotak Apr 23, 2020

lucasponce Apr 23, 2020

jotak Apr 23, 2020

lucasponce Apr 23, 2020

lucasponce Apr 23, 2020

lucasponce Apr 23, 2020 •

edited

Loading

jotak Apr 23, 2020

lucasponce commented Apr 23, 2020

israel-hdez left a comment •

edited

Loading

israel-hdez Apr 23, 2020

jotak Apr 24, 2020

jotak Apr 24, 2020

israel-hdez Apr 23, 2020

jotak commented Apr 24, 2020 •

edited

Loading

jotak commented Apr 24, 2020

jotak commented Apr 27, 2020

aljesusg left a comment

lucasponce left a comment

jotak commented Apr 27, 2020

Allow multiple metrics per chart #87

Allow multiple metrics per chart #87

Conversation

jotak commented Apr 9, 2020 • edited Loading

israel-hdez 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

jotak commented Apr 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasponce Apr 23, 2020 • 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

lucasponce Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasponce commented Apr 23, 2020

israel-hdez left a comment • 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

jotak commented Apr 24, 2020 • edited Loading

jotak commented Apr 24, 2020

jotak commented Apr 27, 2020

aljesusg left a comment

Choose a reason for hiding this comment

lucasponce left a comment

Choose a reason for hiding this comment

jotak commented Apr 27, 2020

jotak commented Apr 9, 2020 •

edited

Loading

lucasponce Apr 23, 2020 •

edited

Loading

lucasponce Apr 23, 2020 •

edited

Loading

israel-hdez left a comment •

edited

Loading

jotak commented Apr 24, 2020 •

edited

Loading