[azure_application_insights] [app_state] Add dimensions, metric_type to the app_state data stream by gpop63 · Pull Request #7550 · elastic/integrations

gpop63 · 2023-08-27T17:42:53Z

What does this PR do?

Dimensions added:

agent.id
azure.dimensions.*
azure.namespace
cloud.region
azure.resource.id
azure.timegrain

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.

Author's Checklist

[ ]

How to test this PR locally

Related issues

Relates Azure Application Insights TSDB Enablement #7616

Screenshots

elasticmachine · 2023-08-27T17:57:41Z

💚 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-10-17T13:45:26.336+0000
Duration: 14 min 3 sec

Test stats 🧪

Test	Results
Failed	0
Passed	16
Skipped	0
Total	16

🤖 GitHub comments

Expand to view the GitHub comments

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

/test : Re-trigger the build.

elasticmachine · 2023-08-27T17:57:49Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`0/0`)	💚
Files	100.0% (`0/0`)	💚
Classes	100.0% (`0/0`)	💚
Methods	57.143% (`4/7`)	👍 32.143
Lines	100.0% (`0/0`)	💚
Conditionals	100.0% (`0/0`)	💚

agithomas · 2023-08-28T07:57:39Z

packages/azure_application_insights/data_stream/app_state/fields/package-fields.yml

-      type: flattened
-      description: >
-        Azure metric dimensions.
+      type: group


What is the purpose behind converting flattened to keyword type? Can this be a breaking change?

Alternate options that can be tried can be found here.

Somehow I think it could be a backporting mistake that this was changed to flattened, for example in beats is used object - https://github.com/elastic/beats/blob/main/x-pack/metricbeat/module/azure/_meta/fields.yml#L53-L58

maybe worth to double-check with codeowners

Yeah, not sure why flattened was used in the data stream that's why I thought changing it makes sense.

Can this be considered as a breaking change?

This may not be an isolated mistake but an intentional change.

I see a similar change in mapping the azure.resource.tags field. Here is how we are mapping the azure.resource.tags field in the Beats module and several metrics integrations:

Metrics Mapping

Integrations Azure Metrics object

Metricbeat Azure module object

Integrations Azure Billing flattened

Integrations Azure App Insights flattened

Integrations Azure App State flattened

I agree with @agithomas that we must double-check whether this is a breaking change.

Switching field types should not create mapping exceptions since the mapping will only change at rollover, however the search behavior may vary due to the different mapping types.

Discussed about this on slack with @gpop63. TLDR; In principle this change looks safe to me, if we are completely sure that this is the list of fields we can have here.

In principle this won't be a breaking change, because keyword fields allow the same operations as individual members of a flattened object (probably more). It could be considered an enhancement because keyword fields probably have better support in Kibana than the flattened type.
It could be breaking only if somehow users put additional fields under azure.dimensions, but I guess there is no reason to think that.

Somehow I think it could be a backporting mistake that this was changed to flattened, for example in beats is used object - https://github.com/elastic/beats/blob/main/x-pack/metricbeat/module/azure/_meta/fields.yml#L53-L58

The flattened type was introduced relatively recently, so this is why you can see it more used in integrations than in Beats. It comes to solve a couple of issues we had with fields with wildcards:

You can have mapping conflicts if there are fields with dots (you cannot store labels.app and labels.app.name as different fields in the same index, because labels.app would need to be mapped as an object and a keyword at the same time). subobjects: false can also help with this issue, but is not supported in packages yet.

You can have "field mappings explosion" if there are too many different fields under this path. This doesn't happen with flattened because it only requires a single mapping for any number of subfields.

But regarding the breaking change, even if it doesn't seem to be actually breaking, please try to do an upgrade from the previous mapping to the proposed here in any case 🙂

@zmoog , are you good with the suggestions from @jsoriano or should we take any more input from other engineers?

@jsoriano tested upgrading and everything is fine, no errors thrown.

@jsorianom, thank you for chiming in and sharing more details and backstory!

@agithomas, after these clarifications, I see no reason to dig more. In addition, @gpop63 tested the mapping transition is smooth.

subobjects: false can also help with this issue, but is not supported in packages yet.

I am working on adding subobjects to package-spec, so we'll have this option soon.

tetianakravchenko · 2023-08-28T08:10:22Z

packages/azure_application_insights/data_stream/app_state/fields/package-fields.yml

-      type: flattened
-      description: >
-        Azure metric dimensions.
+      type: group


Somehow I think it could be a backporting mistake that this was changed to flattened, for example in beats is used object - https://github.com/elastic/beats/blob/main/x-pack/metricbeat/module/azure/_meta/fields.yml#L53-L58

maybe worth to double-check with codeowners

tetianakravchenko · 2023-08-28T08:13:39Z

packages/azure_application_insights/data_stream/app_state/fields/package-fields.yml

+          dimension: true
+          description: The name of the request that was made.

    - name: metrics.*.*


is azure.metrics actually present in the document? in azure_metrics package this field was usually not needed - and was removed

sample document does not include it and metrics are stored under azure.app_state -

integrations/packages/azure_application_insights/data_stream/app_state/sample_event.json

Lines 63 to 75 in 987dd5e

"azure": {

"app_state": {

"end_date": "2021-08-23T14:37:42.472Z",

"requests_count": {

"sum": 4

},

"start_date": "2021-08-23T14:32:42.472Z"

},

"application_id": "42cb59a9-d5be-400b-a5c4-69b0a0026ac6",

"dimensions": {

"request_name": "GET Home/Index",

"request_url_host": "demoappobs.azurewebsites.net"

}

Yeah, metrics could be removed here.

tetianakravchenko · 2023-08-28T08:15:15Z

packages/azure_application_insights/manifest.yml

  - observability
 conditions:
-  kibana.version: "^7.14.0 || ^8.0.0"
+  kibana.version: "^8.9.0"


this change is required to support azure.metrics.*.* as I understand, so can be reverted if azure.metrics.*.* will be removed and introduced with tsdb enablement pr

tetianakravchenko · 2023-08-28T08:19:20Z

packages/azure_application_insights/data_stream/app_state/fields/agent.yml

      ignore_above: 1024
      description: Region in which this host is running.
      example: us-east-1
+      dimension: true


Is this field actually present in the document?
If yes - could you please update sample_event -

integrations/packages/azure_application_insights/data_stream/app_state/sample_event.json

Lines 15 to 17 in 987dd5e

"cloud": {

"provider": "azure"

},

- https://github.com/elastic/integrations/blob/987dd5ec338d1b278ba7002db6e827f87a989798/packages/azure_application_insights/data_stream/app_state/sample_event.json with the latest sample

gpop63 · 2023-08-29T11:10:01Z

Currently, both app_state and app_insights data streams use flattened type for azure.dimensions.

From my understanding there are 2 approaches since flattened type can't be a dimension at the moment:

Keep it flattened, add an ingest pipeline to extract fields and mark them as dimensions
Convert azure.dimensions to group which would provide more flexibility (breaking change?)

I tested upgrading the old package version 1.0.6 to the new one I'm working on 1.1.0 (which changed azure.dimensions to a group) and I did not see any conflicts in the mapping.

agithomas · 2023-09-01T10:09:02Z

since flattened type can't be a dimension at the moment:

Have you tested this : elastic/elasticsearch#94113 (comment)?

This reverts commit 987dd5e.

kaiyan-sheng

the dimensions and metric types look good to me

agithomas

LGTM!

agithomas · 2023-09-11T07:03:06Z

@tetianakravchenko , can you please re-check and validate if it is fine?

tetianakravchenko · 2023-09-11T07:45:50Z

packages/azure_application_insights/data_stream/app_state/fields/fields.yml


    - name: users_authenticated.unique
      type: float
+      metric_type: counter


Are those metrics actual counter? in azure_metrics.container_registry I've seen a different behavior for those type of metrics - https://github.com/tetianakravchenko/integrations/blob/master/packages/azure_metrics/data_stream/container_registry/fields/fields.yml#L4-L11
Those seems to be a counter, but in reality it is not constantly growing:

can you please double-check it?

Yeah you're right, most are actually gauges.

tetianakravchenko · 2023-09-19T12:39:12Z

packages/azure_application_insights/data_stream/app_state/fields/package-fields.yml

-
+      type: group
+      fields:
+        - name: cloud_role_instance


where are those fields dimension.* defined?
As I see part of those dimensions here is the same as in segmentNames https://github.com/elastic/beats/blob/main/x-pack/metricbeat/module/azure/app_insights/data.go#L24-L38
wondering if this list of dimensions is complete

I assume that the list defined in beats could need some updates as well

used for app_state configuration - https://github.com/elastic/beats/blob/main/x-pack/metricbeat/module/azure/app_state/manifest.yml

tetianakravchenko

LGTM!

agithomas · 2023-10-17T08:45:39Z

@gpop63 , are you good to merge this change? Looks like #7259 is waiting on this change.

elasticmachine · 2023-10-17T19:02:33Z

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

gpop63 added 4 commits August 27, 2023 20:33

add dimensions and metric types

080c2ab

update docs

837a33f

bump package version

c3bad76

fix pr id

2524c67

gpop63 self-assigned this Aug 27, 2023

bump kibana version

987dd5e

gpop63 marked this pull request as ready for review August 27, 2023 19:49

gpop63 requested a review from a team as a code owner August 27, 2023 19:49

agithomas reviewed Aug 28, 2023

View reviewed changes

tetianakravchenko reviewed Aug 28, 2023

View reviewed changes

gpop63 mentioned this pull request Aug 30, 2023

Azure Application Insights TSDB Enablement #7616

Closed

10 tasks

agithomas mentioned this pull request Sep 4, 2023

Add support for flattened type dimensions elastic/package-spec#584

Closed

gpop63 mentioned this pull request Sep 7, 2023

[Azure] TSDB enablement - track all metrics data streams #7140

Closed

31 tasks

gpop63 added 3 commits September 7, 2023 15:42

remove metrics from package fields

9c39d88

Revert "bump kibana version"

3bc84e7

This reverts commit 987dd5e.

build package

28dbde4

kaiyan-sheng approved these changes Sep 7, 2023

View reviewed changes

agithomas approved these changes Sep 11, 2023

View reviewed changes

tetianakravchenko reviewed Sep 11, 2023

View reviewed changes

tetianakravchenko mentioned this pull request Sep 11, 2023

[azure_application_insights] [app_insights] Add dimensions and metric_type for metrics #7610

Merged

4 tasks

gpop63 added 3 commits September 12, 2023 01:05

fix metric types

e31ee54

update docs

53b3ad2

Merge remote-tracking branch 'upstream/main' into azure_app-state_tsdb

6c68274

tetianakravchenko reviewed Sep 19, 2023

View reviewed changes

tetianakravchenko approved these changes Sep 19, 2023

View reviewed changes

agithomas mentioned this pull request Oct 17, 2023

[Azure] Azure app state overview dashboard contains flattened data type #7259

Closed

Merge branch 'main' into azure_app-state_tsdb

8d2d658

gpop63 merged commit 39eea2a into elastic:main Oct 17, 2023

andrewkroh added the Integration:azure_application_insights Azure Application Insights Metrics Overview label Jul 22, 2024

	Metrics	Mapping
Integrations	Azure Metrics	`object`
Metricbeat	Azure module	`object`
Integrations	Azure Billing	`flattened`
Integrations	Azure App Insights	`flattened`
Integrations	Azure App State	`flattened`

	"azure": {
	"app_state": {
	"end_date": "2021-08-23T14:37:42.472Z",
	"requests_count": {
	"sum": 4
	},
	"start_date": "2021-08-23T14:32:42.472Z"
	},
	"application_id": "42cb59a9-d5be-400b-a5c4-69b0a0026ac6",
	"dimensions": {
	"request_name": "GET Home/Index",
	"request_url_host": "demoappobs.azurewebsites.net"
	}

Conversation

gpop63 commented Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Screenshots

Uh oh!

elasticmachine commented Aug 27, 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

Uh oh!

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

Uh oh!

🌐 Coverage report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsoriano Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpop63 commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agithomas commented Sep 1, 2023

Uh oh!

kaiyan-sheng left a comment

Choose a reason for hiding this comment

Uh oh!

agithomas left a comment

Choose a reason for hiding this comment

Uh oh!

agithomas commented Sep 11, 2023

Uh oh!

tetianakravchenko Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tetianakravchenko left a comment

Choose a reason for hiding this comment

Uh oh!

agithomas commented Oct 17, 2023

gpop63 commented Aug 27, 2023 •

edited

Loading

elasticmachine commented Aug 27, 2023 •

edited

Loading

elasticmachine commented Aug 27, 2023 •

edited

Loading

jsoriano Sep 4, 2023 •

edited

Loading

gpop63 commented Aug 29, 2023 •

edited

Loading

tetianakravchenko Sep 11, 2023 •

edited

Loading