[Istio] Add metric type to fields #6889

constanca-m · 2023-07-10T08:24:58Z

What does this PR do?

Add metric type to metric fields for TSDB migration.

Histogram object does not have a metric type because it is not working - metric type gauge not appearing on the mapping:

 "dynamic_templates": [ { "istio.proxy.metrics.*.value": { "path_match": "istio.proxy.metrics.*.value", "mapping": { "time_series_metric": "gauge", "type": "double" } } }, { "istio.proxy.metrics.*.counter": { "path_match": "istio.proxy.metrics.*.counter", "mapping": { "time_series_metric": "counter", "type": "double" } } }, { "istio.proxy.metrics.*.rate": { "path_match": "istio.proxy.metrics.*.rate", "mapping": { "time_series_metric": "gauge", "type": "double" } } }, { "istio.proxy.metrics.*.histogram": { "path_match": "istio.proxy.metrics.*.histogram", "mapping": { "type": "histogram" } } },

Additionally, enabling TSDB with these metric types will cause a visualization to break since it is using an unsupported aggregation for type counter:

This is not a blocker to this PR, because TSDB is not yet enabled.

Extra: Proof that counter rate is a gauge:

Checklist

I have reviewed tips for building integrations and this pull request is aligned with them.
I have verified that all data streams collect metrics or logs.
I have added an entry to my package's changelog.yml file.
I have verified that Kibana version constraints are current according to guidelines.

Related issues

Relates to #6567.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

elasticmachine · 2023-07-10T08:36:32Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-07-18T08:28:52.788+0000
Duration: 15 min 4 sec

Test stats 🧪

Test	Results
Failed	0
Passed	14
Skipped	0
Total	14

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

elasticmachine · 2023-07-10T09:13:47Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`1/1`)	💚
Files	100.0% (`3/3`)	💚 5.0
Classes	100.0% (`3/3`)	💚 5.0
Methods	90.0% (`27/30`)	👍 3.781
Lines	97.472% (`347/356`)	👍 8.072
Conditionals	100.0% (`0/0`)	💚

ChrsMark

Hey @constanca-m could you please add more details in the PR's description about what impact these changes will have? Will the package be broken? From what is mentioned in the description this is not clear to me. Maybe I miss context here but still I think that PRs should be self explanatory. Any links that would help here should be included in the PR's description.

ChrsMark · 2023-07-10T09:57:33Z

packages/istio/changelog.yml

@@ -1,4 +1,9 @@
 # newer versions go on top
+- version: "0.3.1"


I think this should be bumped up to 0.4.0 since it's not a bugfix but an enhancement.

Will the package be broken?

The package will not be broken. Enabling TSDB with this change will break one of the visualizations of the predefined dashboard. But now, TSDB is not enabled, so merging this PR would not make any difference on that visualization.

I think this should be bumped up to 0.4.0 since it's not a bugfix but an enhancement.

I bumped it to 0.3.1 since 0.3.0 has another TSDB enhancement and this logic has been followed for every TSDB changes on the packages. But I could update it to 0.4.0 if you still think it's better?

I think 0.4.0 is more accurate in general.
However do you have a specific plan that you folks follow in all TSDB changes? If that's the case is this decided/documented or it's just a convention?

it is just a convention, it is not documented anywhere

ChrsMark · 2023-07-10T10:39:31Z

Histogram object does not have a metric type because it is not working - metric type gauge not appearing on the mapping:

Can we be more verbose on this? Why it's not working? Do we need a follow-up fix for this? Reading this makes me think that sth is broken but still do not know what is it and if related with this change.

Additionally, enabling TSDB with these metric types will cause a visualization to break since it is using an unsupported aggregation for type counter:

Can you add a note here that it's not an issue for this PR since it does not enable TSDB?

Extra: Proof that counter rate is a gauge:

What is the context of this? Can you add more context for what is this one related to please?

constanca-m · 2023-07-10T11:04:15Z

Can we be more verbose on this? Why it's not working? Do we need a follow-up fix for this? Reading this makes me think that sth is broken but still do not know what is it and if related with this change.

I will bring it up tomorrow on TSDB meeting. I am also not sure why histogram types are being "ignored" for this purpose. The dynamic templating was an issue solved not long ago: elastic/kibana#155004 and we can see in this merged PR that every metric was changed apart from the histograms.

Can you add a note here that it's not an issue for this PR since it does not enable TSDB?

Yes, I will add it.

What is the context of this? Can you add more context for what is this one related to please?

Those metrics are counter rates and I though at first that since they are counter rates, the metric type would be counter. Then I checked to confirm how they change over time and realized they were a gauge. That's all the context, it is just a proof to the reviewer in case they have the same doubt that I had.

tetianakravchenko · 2023-07-12T14:35:04Z

packages/istio/data_stream/istiod_metrics/fields/fields.yml

 object_type: keyword
 description: |
 Istiod metric labels
 - name: istio.istiod.metrics.*.value


@constanca-m can you please share as well the screenshot of the data view for those fields? and the mapping?

Yes, the mapping is on the description.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

elasticmachine · 2023-07-18T15:24:17Z

Package istio - 0.3.1 containing this change is available at https://epr.elastic.co/search?package=istio

Add metric type.

40746e9

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

constanca-m added the Integration:istio Istio label Jul 10, 2023

constanca-m self-assigned this Jul 10, 2023

constanca-m requested a review from a team as a code owner July 10, 2023 08:24

constanca-m requested review from ChrsMark and tetianakravchenko July 10, 2023 08:25

constanca-m mentioned this pull request Jul 10, 2023

[Istio] Track TSDB process #6567

Closed

4 tasks

update changelog.

77872e9

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

fix metric type.

0596018

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

ChrsMark reviewed Jul 10, 2023

View reviewed changes

constanca-m requested a review from ChrsMark July 11, 2023 07:04

tetianakravchenko reviewed Jul 12, 2023

View reviewed changes

constanca-m requested a review from tetianakravchenko July 18, 2023 08:23

Update kibana version.

0d83738

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

tetianakravchenko approved these changes Jul 18, 2023

View reviewed changes

constanca-m merged commit c7df056 into elastic:main Jul 18, 2023

constanca-m deleted the istio-add-metrics branch July 18, 2023 15:00

constanca-m mentioned this pull request Aug 30, 2023

[Istio] Revert Istio metric type to object #7607

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Istio] Add metric type to fields #6889

[Istio] Add metric type to fields #6889

Uh oh!

constanca-m commented Jul 10, 2023 •

edited

Loading

elasticmachine commented Jul 10, 2023 •

edited

Loading

Build stats

Test stats 🧪

elasticmachine commented Jul 10, 2023 •

edited

Loading

ChrsMark left a comment

ChrsMark Jul 10, 2023

constanca-m Jul 10, 2023

ChrsMark Jul 10, 2023

constanca-m Jul 10, 2023

ChrsMark commented Jul 10, 2023

constanca-m commented Jul 10, 2023 •

edited

Loading

tetianakravchenko Jul 12, 2023

constanca-m Jul 12, 2023

elasticmachine commented Jul 18, 2023

Labels

4 participants

[Istio] Add metric type to fields #6889

[Istio] Add metric type to fields #6889

Uh oh!

Conversation

constanca-m commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist

Related issues

elasticmachine commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

elasticmachine commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

ChrsMark left a comment

Choose a reason for hiding this comment

ChrsMark Jul 10, 2023

Choose a reason for hiding this comment

constanca-m Jul 10, 2023

Choose a reason for hiding this comment

ChrsMark Jul 10, 2023

Choose a reason for hiding this comment

constanca-m Jul 10, 2023

Choose a reason for hiding this comment

ChrsMark commented Jul 10, 2023

constanca-m commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

tetianakravchenko Jul 12, 2023

Choose a reason for hiding this comment

constanca-m Jul 12, 2023

Choose a reason for hiding this comment

elasticmachine commented Jul 18, 2023

Labels

4 participants

constanca-m commented Jul 10, 2023 •

edited

Loading

elasticmachine commented Jul 10, 2023 •

edited

Loading

elasticmachine commented Jul 10, 2023 •

edited

Loading

constanca-m commented Jul 10, 2023 •

edited

Loading