Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

azure logs: add ECS mapping for event.duration #11104

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

zmoog
Copy link
Contributor

@zmoog zmoog commented Sep 11, 2024

Proposed commit message

Convert the event.duration field value to the long type.

Users reported mapping exceptions due to event.duration string values causing field mapping as keyword instead of long. See #10848 to learn more.

Elasticsearch maps a field as a keyword if it has a string value. This happens even on stack versions 8.13+ because ecs@mappings does not perform type coercion.

By converting the event.duration field values to the long type, we ensure Elasticsearch uses the expected ECS field mapping as long.

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

@zmoog zmoog added Integration:azure Azure Logs bugfix Pull request that fixes a bug issue labels Sep 11, 2024
@zmoog zmoog self-assigned this Sep 11, 2024
@elasticmachine
Copy link

🚀 Benchmarks report

Package azure 👍(5) 💚(4) 💔(2)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
signinlogs 2450.98 2016.13 -434.85 (-17.74%) 💔
firewall_logs 1694.92 1398.6 -296.32 (-17.48%) 💔

To see the full report comment with /test benchmark fullreport

@zmoog zmoog marked this pull request as ready for review September 11, 2024 22:26
@zmoog zmoog requested review from a team as code owners September 11, 2024 22:26
@andrewkroh andrewkroh added the Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team [elastic/obs-infraobs-integrations] label Sep 11, 2024
Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest also

diff --git a/packages/azure/data_stream/activitylogs/elasticsearch/ingest_pipeline/default.yml b/packages/azure/data_stream/activitylogs/elasticsearch/ingest_pipeline/default.yml
index e5e7c52bbd..fcdc5b2eb3 100644
--- a/packages/azure/data_stream/activitylogs/elasticsearch/ingest_pipeline/default.yml
+++ b/packages/azure/data_stream/activitylogs/elasticsearch/ingest_pipeline/default.yml
@@ -72,10 +72,23 @@ processors:
       field: azure.activitylogs.durationMs
       target_field: event.duration
       ignore_missing: true
+  - convert:
+      field: event.duration
+      tag: convert_event_duration
+      type: long
+      ignore_missing: true
+      on_failure:
+        - remove:
+            field: event.duration
+        - append:
+            field: error.message
+            value: 'Processor {{{_ingest.on_failure_processor_type}}} with tag {{{_ingest.on_failure_processor_tag}}} in pipeline {{{_ingest.pipeline}}} failed with message: {{{_ingest.on_failure_message}}}'
   - script:
       lang: painless
-      source: if (ctx.event.duration!= null) {ctx.event.duration = ctx.event.duration
-        * params.param_nano;}
+      source: >
+        if (ctx.event.duration != null) {
+          ctx.event.duration = ctx.event.duration * params.param_nano;
+        }
       params:
         param_nano: 1000000
       ignore_failure: true

and changelog/manifest updates.

@zmoog
Copy link
Contributor Author

zmoog commented Sep 12, 2024

Hey @efd6, thanks for suggesting the convert processor.

I was looking for a temporarily quick fix for the mapping, adding back fields/ecs.yml file, but converting it solves the problem. I can remove the fields/ecs.yml and only leverage ecs@mappings.

The field value must be a `long` to align with ECS and correctly
leverage the ecs@mappings component template.
@zmoog zmoog enabled auto-merge (squash) September 17, 2024 09:28
@zmoog
Copy link
Contributor Author

zmoog commented Sep 18, 2024

/test

@elasticmachine
Copy link

elasticmachine commented Sep 18, 2024

💔 Build Failed

Failed CI Steps

History

cc @zmoog

@@ -72,10 +72,23 @@ processors:
field: azure.activitylogs.durationMs
target_field: event.duration
ignore_missing: true
- convert:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we adding back the mapping to ecs.yml instead of doing this ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we have seen that the conversion is not effective in communicating that the type should be a long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes a bug issue Integration:azure Azure Logs Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team [elastic/obs-infraobs-integrations]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants