[Azure] Migrate Azure logs package to adopt ecs@mappings by harnish-crest-data · Pull Request #10224 · elastic/integrations

harnish-crest-data · 2024-06-24T12:20:50Z

Enhancement

Proposed commit message

Command

  go run github.com/andrewkroh/go-examples/ecs-update@014b35dfe4c9832b51e7c909a39a48257d6a005d \
    -ecs-version=8.11.0 \
    -ecs-git-ref=v8.11.0 \
    -fields-yml-drop-ecs \
    -kibana-version=^8.13.0 \
    -drop-import-mappings \
    packages/azure

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.

ishleenk17 · 2024-06-24T15:29:08Z

As part of migrating azure package to ecs@mappings, the below 2 datastreams have pipeline failures.

Firewall Logs:

Issue:
azure/firewall_logs test-dnsproxyrules-raw.log:
[0] parsing field value failed: field "dns.header_flags"'s value "QR" is not one of the expected values (AA, TC, RD, RA, AD, CD, DO)
The allowed values are [these](https://www.elastic.co/guide/en/ecs/current/ecs-dns.html#field-dns-header-flags). 

Solution: Considering that qr can't be a header flag, change the pipeline logs with appropriate flags

Platform Logs:

Issue: 
azure/platformlogs test-platformlogs-edgecases.log:
[0] parsing field value failed: field "event.outcome"'s value "succeeded" is not one of the allowed values (failure, success, unknown)
azure/platformlogs test-platformlogs-raw.log:
[0] parsing field value failed: field "event.outcome"'s value "succeeded" is not one of the allowed values (failure, success, unknown)

Azure platformlogs, the value for event.outcome is being set from the following processors.  [Refer](https://github.com/elastic/integrations/blob/main/packages/azure/data_stream/platformlogs/elasticsearch/ingest_pipeline/default.yml#L174-L188)

  - convert:
      field: azure.platformlogs.Status
      target_field: event.outcome
      type: string
      if: "ctx?.event?.outcome == null && ctx?.azure?.platformlogs?.Status != null && ctx?.azure?.platformlogs?.Status instanceof String && ['success', 'failure', 'unknown', 'Succeeded', 'Failed'].contains(ctx.azure?.platformlogs?.Status)"

Solution: Remove the Succeeded, Failed. But, are those codes coming from the azure platform logs directly. Removing these might break things if user is already using these values.

cc: @zmoog, @muthu-mps

muthu-mps · 2024-06-25T05:12:25Z

As part of migrating azure package to ecs@mappings, the below 2 datastreams have pipeline failures.

Firewall Logs:

Issue:
azure/firewall_logs test-dnsproxyrules-raw.log:
[0] parsing field value failed: field "dns.header_flags"'s value "QR" is not one of the expected values (AA, TC, RD, RA, AD, CD, DO)
The allowed values are [these](https://www.elastic.co/guide/en/ecs/current/ecs-dns.html#field-dns-header-flags). 

Solution: Considering that qr can't be a header flag, change the pipeline logs with appropriate flags

The QR is one of the header values for the Azure Firewall log. We cannot ignore this value for the Firewall logs.
Not much information available, But you can find the details on the QR header value here.

muthu-mps · 2024-06-25T05:20:14Z

Platform Logs:

Issue: 
azure/platformlogs test-platformlogs-edgecases.log:
[0] parsing field value failed: field "event.outcome"'s value "succeeded" is not one of the allowed values (failure, success, unknown)
azure/platformlogs test-platformlogs-raw.log:
[0] parsing field value failed: field "event.outcome"'s value "succeeded" is not one of the allowed values (failure, success, unknown)

Azure platformlogs, the value for event.outcome is being set from the following processors.  [Refer](https://github.com/elastic/integrations/blob/main/packages/azure/data_stream/platformlogs/elasticsearch/ingest_pipeline/default.yml#L174-L188)

  - convert:
      field: azure.platformlogs.Status
      target_field: event.outcome
      type: string
      if: "ctx?.event?.outcome == null && ctx?.azure?.platformlogs?.Status != null && ctx?.azure?.platformlogs?.Status instanceof String && ['success', 'failure', 'unknown', 'Succeeded', 'Failed'].contains(ctx.azure?.platformlogs?.Status)"

Solution: Remove the Succeeded, Failed. But, are those codes coming from the azure platform logs directly. Removing these might break things if user is already using these values.

cc: @zmoog, @muthu-mps

In this case, When looking into the different Azure log integrations the event.outcome is not consistent from the Azure logs and when we take the sign-in logs as example the status_code:1 is considered as event.outcome: success and the status_code:0 as event.outcome:failure.
Ideally, we should convert the succeeded and failed values to success and failure respectively in the ingest pipeline.

Impact

By changing the status values if the user is using them in the filter they won't see any results for succeeded or failed. Instead they will be added to the success or failure. This is fine IMO.
If the chart groups values by event.outcome this will not include the succeeded value anymore.

@zmoog - What do you think?

ishleenk17 · 2024-06-26T05:44:57Z

The QR is one of the header values for the Azure Firewall log. We cannot ignore this value for the Firewall logs.

@harnish-elastic : To add this value to the expected values of DNS header flags, let's override the expected values of this field in ecs.yml, something like this example. Let's see if things work.

Ideally, we should convert the succeeded and failed values to success and failure respectively in the ingest pipeline.

@zmoog @muthu-mps : The question here is even if you convert succeeded -> success internally, will the user be expecting this status to be succeeded only?
IMO, the time it communicates the same meaning, it should not be a big deal. We can change these to expected values in the ingest pipeline. succeeded->success and failed->failure

Another option is overriding the event.outcome in ecs.yml like this example

harnish-crest-data · 2024-06-26T06:15:13Z

The QR is one of the header values for the Azure Firewall log. We cannot ignore this value for the Firewall logs.

@harnish-elastic : To add this value to the expected values of DNS header flags, let's override the expected values of this field in ecs.yml, something like this example. Let's see if things work.

@ishleenk17 I have checked by adding expected values like below and it worked!

- name: dns.header_flags
  external: ecs
  expected_values:
    - AA
    - TC
    - RD
    - RA
    - AD
    - CD
    - DO
    - QR

@zmoog @muthu-mps The question here is, is there a chance that we can also get the values out of the above mentioned list from azure firewall_logs?

ishleenk17 · 2024-06-27T07:29:24Z

@tommyers-elastic : As part of the PR, you moved the Geo ECS fields from ecs.yml to fields.yml.
As part migrating to ecs@mappings, ideally we should remove these fields as well and use the default ecs definition of geo fields. DO you see an issue here ?

ishleenk17 · 2024-07-03T06:01:11Z

@zmoog @muthu-mps The question here is, is there a chance that we can also get the values out of the above mentioned list from azure firewall_logs?

@muthu-mps @zmoog : Are there any concerns here else we will move on with these option overriding the default values for now.

@tommyers-elastic : As part of the https://github.com/elastic/integrations/pull/8050, you moved the Geo ECS fields from ecs.yml to fields.yml.
As part migrating to ecs@mappings, ideally we should remove these fields as well and use the default ecs definition of geo fields. DO you see an issue here ?

@tommyers-elastic : WDYT ?

muthu-mps · 2024-07-03T06:19:43Z

The QR is one of the header values for the Azure Firewall log. We cannot ignore this value for the Firewall logs.

@harnish-elastic : To add this value to the expected values of DNS header flags, let's override the expected values of this field in ecs.yml, something like this example. Let's see if things work.

@ishleenk17 I have checked by adding expected values like below and it worked!
- name: dns.header_flags
  external: ecs
  expected_values:
    - AA
    - TC
    - RD
    - RA
    - AD
    - CD
    - DO
    - QR
@zmoog @muthu-mps The question here is, is there a chance that we can also get the values out of the above mentioned list from azure firewall_logs?

So far we have not seen any other values other than QR.

muthu-mps · 2024-07-03T06:28:27Z

@zmoog @muthu-mps The question here is, is there a chance that we can also get the values out of the above mentioned list from azure firewall_logs?

@muthu-mps @zmoog : Are there any concerns here else we will move on with these option overriding the default values for now.

These are the common ones and we can go-ahead with this values.

ishleenk17 · 2024-07-03T06:45:36Z

@zmoog @muthu-mps The question here is, is there a chance that we can also get the values out of the above mentioned list from azure firewall_logs?
@muthu-mps @zmoog : Are there any concerns here else we will move on with these option overriding the default values for now.

These are the common ones and we can go-ahead with this values.

@harnish-elastic : Lets proceed with these then

muthu-mps · 2024-07-03T07:30:01Z

The QR is one of the header values for the Azure Firewall log. We cannot ignore this value for the Firewall logs.

@harnish-elastic : To add this value to the expected values of DNS header flags, let's override the expected values of this field in ecs.yml, something like this example. Let's see if things work.

Ideally, we should convert the succeeded and failed values to success and failure respectively in the ingest pipeline.

@zmoog @muthu-mps : The question here is even if you convert succeeded -> success internally, will the user be expecting this status to be succeeded only? IMO, the time it communicates the same meaning, it should not be a big deal. We can change these to expected values in the ingest pipeline. succeeded->success and failed->failure

Another option is overriding the event.outcome in ecs.yml like this example

I don't think user would expect the succeeded value. I would also like the hear from @zmoog based on the past experiences.

harnish-crest-data · 2024-07-03T07:34:35Z

@zmoog @muthu-mps The question here is, is there a chance that we can also get the values out of the above mentioned list from azure firewall_logs?
@muthu-mps @zmoog : Are there any concerns here else we will move on with these option overriding the default values for now.

These are the common ones and we can go-ahead with this values.

@harnish-elastic : Lets proceed with these then

Changes are already pushed!

zmoog · 2024-07-03T10:51:48Z

@zmoog @muthu-mps The question here is, is there a chance that we can also get the values out of the above mentioned list from azure firewall_logs?
@muthu-mps @zmoog : Are there any concerns here else we will move on with these option overriding the default values for now.

These are the common ones and we can go-ahead with this values.

@harnish-elastic : Lets proceed with these then

Changes are already pushed!

I'm late to the party, but it's +1 from me.

The question here is even if you convert succeeded -> success internally, will the user be expecting this status to be succeeded only? IMO, the time it communicates the same meaning, it should not be a big deal. We can change these to expected values in the ingest pipeline. succeeded->success and failed->failure
Another option is overriding the event.outcome in ecs.yml like this example

I don't think user would expect the succeeded value. I would also like the hear from @zmoog based on the past experiences.

We usually try to maintain as much backward compatibility as possible. However, the current event.outcome field values are not the expected ones per ECS.

Platform logs are a semi-generic data stream. The Azure Native ISV uses platform logs as the default data stream when it doesn't have a specific routing.

Aligning the event.outcome field to the ECS expected values (succeeded->success and failed->failure) feels like a breaking change. Since platform logs are a generic data stream, users may use this field to check for event outcomes on various log categories, in ways that go beyond what the integration offers.

To take this as the opportunity to align the expected values (succeeded->success and failed->failure) for the event.outcome field, we probably need to highlight this release contains breaking changes, like bumping package version from v1 to v2. I would take this opportunity.

@jsoriano, what is the best practice to communicate breaking changes in integration releases?

jsoriano · 2024-07-03T10:59:09Z

@jsoriano, what is the best practice to communicate breaking changes in integration releases?

As you mention we try to avoid them, but if needed, you could bump the major version, and include a "breaking-change" entry in the changelog.
I think this would be more than enough for this case where we are slightly changing some values for coherence with ECS and other integrations.

zmoog · 2024-07-03T11:12:46Z

@jsoriano, what is the best practice to communicate breaking changes in integration releases?

As you mention we try to avoid them, but if needed, you could bump the major version, and include a "breaking-change" entry in the changelog. I think this would be more than enough for this case where we are slightly changing some values for coherence with ECS and other integrations.

Yeah, I try to avoid breaking changes as much as possible. In this case, aligning the event.outcome field value to ECS and other integrations is probably worth the cost.

To properly mark this change as breaking, do we need to set type as in the following example?

- version: "0.4.0"
  changes:
    - description: added anonymous auth config, replaced some RUM config
      type: breaking-change
      link: https://github.com/elastic/apm-server/pull/5623

jsoriano · 2024-07-03T11:46:46Z

In this case, aligning the event.outcome field value to ECS and other integrations is probably worth the cost.

Agree. And if any user is affected by this change the adaptation would be easy, by modifying queries or dashboards to use the new values.

To properly mark this change as breaking, do we need to set type as in the following example?

Correct.

ishleenk17 · 2024-07-05T04:30:54Z

To properly mark this change as breaking, do we need to set type as in the following example?

@harnish-elastic : Let's take care of this.
Please resolve the CI error

…led" to "failure"

elasticmachine · 2024-07-05T10:57:12Z

🚀 Benchmarks report

Package `azure` 👍(7) 💚(2) 💔(2)

Expand to view

Data stream	Previous EPS	New EPS	Diff (%)	Result
`firewall_logs`	1976.28	1547.99	-428.29 (-21.67%)	💔
`platformlogs`	4694.84	3508.77	-1186.07 (-25.26%)	💔

To see the full report comment with /test benchmark fullreport

harnish-crest-data · 2024-07-05T11:14:20Z

To properly mark this change as breaking, do we need to set type as in the following example?

@harnish-elastic : Let's take care of this. Please resolve the CI error

Updated, thanks!

CI is getting failed due to SonarQube Code Analysis which can be ignored as per this comment!

muthu-mps · 2024-07-05T11:42:58Z

packages/azure/changelog.yml

@@ -1,3 +1,11 @@
+- version: "1.12.0"
+  changes:
+    - description: ECS version updated to 8.11.0. Update the kibana constraint to ^8.13.0. Modified the field definitions to remove ECS fields made redundant by the ecs@mappings component template.


This changelog has multiple updates, Can we have separate descriptions for each.

Update ECS version to 8.11.0

Update the kibana constraint to ^8.13.0 to adopt ecs@mappings component template.

No need to add this sentence in the changelog entry,
Modified the field definitions to remove ECS fields made redundant by the ecs@mappings component template.

This is the text that the automation writes.

Separate entries sounds like a reasonable enhancement to make to ecs-update. The related code is at https://github.com/andrewkroh/go-examples/blob/7bd2d3beac526b97b7734b2b51433d4be54d0fb6/ecs-update/ecs-update.go#L343 if anyone wants to contribute.

muthu-mps · 2024-07-05T11:46:49Z

@harnish-elastic - After making the changes, Did we test the data collection for any of the Azure logs integration?

efd6

application_gateway and firewall_logs LGTM

efd6 · 2024-07-07T23:42:22Z

packages/azure/changelog.yml

@@ -1,3 +1,11 @@
+- version: "1.12.0"
+  changes:
+    - description: ECS version updated to 8.11.0. Update the kibana constraint to ^8.13.0. Modified the field definitions to remove ECS fields made redundant by the ecs@mappings component template.


This is the text that the automation writes.

efd6 · 2024-07-07T23:53:58Z

packages/azure/data_stream/firewall_logs/fields/ecs.yml

+    - AD
+    - CD
+    - DO
+    - QR


In coredns this flag is removed.

integrations/packages/coredns/data_stream/log/elasticsearch/ingest_pipeline/default.yml

Lines 98 to 108 in af56b28

- script:

lang: painless

ignore_failure: true

description: remove QR header since it is not allowed per ECS spec in 8.4.0

if: ctx.dns?.header_flags != null && ctx.dns.header_flags instanceof List

source: |

for (int i=0; i<ctx.dns.header_flags.length; i++) {

if (ctx.dns.header_flags[i] == 'QR') {

ctx.dns.header_flags.remove(i);

}

}

ISTM that either it should be removed, used to otherwise colour the document or be added to the allowed set in ECS (or a combination of these).

QR (Query/Rsponse) seems to be a valid DNS header flag.
IMO, we should not remove it here, rather have it added in the allowed list.
@jsoriano

ishleenk17

Looks good!
Please address others' comments if pending.

harnish-crest-data · 2024-07-08T05:12:42Z

@harnish-elastic - After making the changes, Did we test the data collection for any of the Azure logs integration?

I have done the pipeline tests as Azure package is not having the system tests for platformlogs data stream. Pipeline tests are working as expected!

niraj-elastic

LGTM

muthu-mps

LGTM!

zmoog

LGTM, only a minor fix to the repo name in the changelog link.

packages/azure/changelog.yml

Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>

elastic-sonarqube · 2024-07-08T16:29:12Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
93.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

elasticmachine · 2024-07-08T16:29:20Z

💚 Build Succeeded

Buildkite Build
Commit: 8d2a674

History

💚 Build #13299 succeeded 3ba83d1
💔 Build #12888 failed f452d01
💔 Build #12803 failed b4138a6
💔 Build #12777 failed 1ed4664
💔 Build #12774 failed 0fc5d53
💔 Build #12770 failed 0a68880

cc @harnish-elastic

elasticmachine · 2024-07-08T16:46:57Z

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

[azure] - change to ECS version git@v8.11.0

0a68880

harnish-crest-data self-assigned this Jun 24, 2024

harnish-crest-data and others added 2 commits June 24, 2024 18:07

update changelog.yml

0fc5d53

update readme

1ed4664

harnish-crest-data changed the title ~~[Azure] Migrate ECS version to git@v8.11.0~~ [Azure] Migrate Azure package to ecs@mappings Jun 25, 2024

update readme

b4138a6

add expected values to dns.header field in firewall_logs data stream

f452d01

Updated "event.outcome" values from "Succeeded" to "success" and "Fai…

3ba83d1

…led" to "failure"

harnish-crest-data marked this pull request as ready for review July 5, 2024 11:11

harnish-crest-data requested review from a team as code owners July 5, 2024 11:11

harnish-crest-data requested review from ishleenk17, jsoriano, muthu-mps, niraj-elastic and zmoog July 5, 2024 11:11

muthu-mps changed the title ~~[Azure] Migrate Azure package to ecs@mappings~~ [Azure] Migrate Azure logs package to adopt ecs@mappings Jul 5, 2024

muthu-mps reviewed Jul 5, 2024

View reviewed changes

efd6 approved these changes Jul 8, 2024

View reviewed changes

ishleenk17 approved these changes Jul 8, 2024

View reviewed changes

harnish-crest-data requested a review from muthu-mps July 8, 2024 05:14

niraj-elastic approved these changes Jul 8, 2024

View reviewed changes

muthu-mps approved these changes Jul 8, 2024

View reviewed changes

zmoog approved these changes Jul 8, 2024

View reviewed changes

packages/azure/changelog.yml Outdated Show resolved Hide resolved

Update packages/azure/changelog.yml

8d2a674

Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>

ishleenk17 merged commit b99f669 into elastic:main Jul 8, 2024

harnish-crest-data mentioned this pull request Jul 9, 2024

packages/salesforce: add dashboards #10341

Merged

4 tasks

andrewkroh added the Integration:azure Azure Logs label Jul 22, 2024

	- script:
	lang: painless
	ignore_failure: true
	description: remove QR header since it is not allowed per ECS spec in 8.4.0
	if: ctx.dns?.header_flags != null && ctx.dns.header_flags instanceof List
	source: \|
	for (int i=0; i<ctx.dns.header_flags.length; i++) {
	if (ctx.dns.header_flags[i] == 'QR') {
	ctx.dns.header_flags.remove(i);
	}
	}

Conversation

harnish-crest-data commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed commit message

Checklist

Uh oh!

ishleenk17 commented Jun 24, 2024

Uh oh!

muthu-mps commented Jun 25, 2024

Uh oh!

muthu-mps commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Impact

Uh oh!

ishleenk17 commented Jun 26, 2024

Uh oh!

harnish-crest-data commented Jun 26, 2024

Uh oh!

ishleenk17 commented Jun 27, 2024

Uh oh!

ishleenk17 commented Jul 3, 2024

Uh oh!

muthu-mps commented Jul 3, 2024

Uh oh!

muthu-mps commented Jul 3, 2024

Uh oh!

ishleenk17 commented Jul 3, 2024

Uh oh!

muthu-mps commented Jul 3, 2024

Uh oh!

harnish-crest-data commented Jul 3, 2024

Uh oh!

zmoog commented Jul 3, 2024

Uh oh!

jsoriano commented Jul 3, 2024

Uh oh!

zmoog commented Jul 3, 2024

Uh oh!

jsoriano commented Jul 3, 2024

Uh oh!

ishleenk17 commented Jul 5, 2024

Uh oh!

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

Uh oh!

🚀 Benchmarks report

Package azure 👍(7) 💚(2) 💔(2)

Uh oh!

harnish-crest-data commented Jul 5, 2024

Uh oh!

muthu-mps Jul 5, 2024

Choose a reason for hiding this comment

Uh oh!

efd6 Jul 7, 2024

Choose a reason for hiding this comment

Uh oh!

andrewkroh Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

muthu-mps commented Jul 5, 2024

Uh oh!

efd6 left a comment

Choose a reason for hiding this comment

Uh oh!

efd6 Jul 7, 2024

Choose a reason for hiding this comment

Uh oh!

efd6 Jul 7, 2024

Choose a reason for hiding this comment

Uh oh!

ishleenk17 Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

ishleenk17 left a comment

Choose a reason for hiding this comment

Uh oh!

harnish-crest-data commented Jul 8, 2024

Uh oh!

niraj-elastic left a comment

Choose a reason for hiding this comment

Uh oh!

harnish-crest-data commented Jun 24, 2024 •

edited

Loading

muthu-mps commented Jun 25, 2024 •

edited

Loading

elasticmachine commented Jul 5, 2024 •

edited

Loading

Package `azure` 👍(7) 💚(2) 💔(2)