Skip to content

graphactivitylogs: Fix client|source.geo.location mapping#11102

Merged
zmoog merged 3 commits intoelastic:mainfrom
zmoog:zmoog/fix-azire-graphactivitylogs-geo-location
Sep 17, 2024
Merged

graphactivitylogs: Fix client|source.geo.location mapping#11102
zmoog merged 3 commits intoelastic:mainfrom
zmoog:zmoog/fix-azire-graphactivitylogs-geo-location

Conversation

@zmoog
Copy link
Copy Markdown
Contributor

@zmoog zmoog commented Sep 11, 2024 •
edited
Loading

Proposed commit message

Align client|source.geo.location fields to ECS mapping.

Users reported mapping exceptions due to Elasticsearch mapping theclient|source.geo.location fields as object instead of geo_point. See #10848 for more.

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

@andrewkroh andrewkroh added the Integration:azure Azure Logs label Sep 11, 2024
@zmoog zmoog self-assigned this Sep 11, 2024
@zmoog zmoog added the bug Something isn't working, use only for issues label Sep 11, 2024
@zmoog
Copy link
Copy Markdown
Contributor Author

zmoog commented Sep 11, 2024 •
edited
Loading

According to the Client Fields and Source Fields ECS reference, we should map client.geo.location and source.geo.location using the geo_point type.

However, the packages/azure/data_stream/graphactivitylogs/fields/ecs.yml has the following mapping:

- name: client.geo.location.lat
  external: ecs
- name: client.geo.location.lon
  external: ecs
- name: source.geo.location.lat
  external: ecs
- name: source.geo.location.lon
  external: ecs

That causes Elasticsearch to map the client.geo.location and source.geo.location fields as object.

We should probably change the mapping to:

- name: client.geo.location
  external: ecs
- name: source.geo.location
  external: ecs

To align these fields with ECS and produce the expected geo_point mapping.

@kcreddy, are there specific reasons to use the *.lat|.lon mapping?

@andrewkroh andrewkroh added bugfix Pull request that fixes a bug issue and removed bug Something isn't working, use only for issues labels Sep 11, 2024
@elasticmachine
Copy link
Copy Markdown

elasticmachine commented Sep 11, 2024

🚀 Benchmarks report

Package azure 👍(6) 💚(4) 💔(1)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
provisioning 3225.81 2398.08 -827.73 (-25.66%) 💔

To see the full report comment with /test benchmark fullreport

@zmoog zmoog mentioned this pull request Sep 11, 2024
Closed
@zmoog zmoog marked this pull request as ready for review September 11, 2024 22:36
@zmoog zmoog requested review from a team as code owners September 11, 2024 22:36
@andrewkroh andrewkroh added the Team:Security-Service Integrations Security Service Integrations team [elastic/security-service-integrations] label Sep 11, 2024
@elasticmachine
Copy link
Copy Markdown

elasticmachine commented Sep 11, 2024

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

efd6
efd6 reviewed Sep 11, 2024
View reviewed changes
Copy link
Copy Markdown
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.

This needs a changelog and manifest update.

muthu-mps
muthu-mps approved these changes Sep 12, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@muthu-mps muthu-mps left a comment

Choose a reason for hiding this comment

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

code owner approval.

kcreddy
kcreddy approved these changes Sep 12, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@kcreddy kcreddy left a comment

Choose a reason for hiding this comment

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

@kcreddy, are there specific reasons to use the *.lat|.lon mapping?

I don't see one. Must be a miss from previous datastreams which missed this fix.
Thanks for fixing 👍🏼

andrewkroh
andrewkroh reviewed Sep 12, 2024
View reviewed changes
packages/azure/changelog.yml Outdated
- version: "1.14.1"
changes:
- description: Fix [client|source].geo.location ECS field mapping
type: enhancement
Copy link
Copy Markdown
Member

@andrewkroh andrewkroh Sep 12, 2024

Choose a reason for hiding this comment

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

You labeled the PR as a bug and the patch version was incremented so all signals suggest this should be type: bugfix instead of enhancement. Can you confirm the intention.

Copy link
Copy Markdown
Contributor Author

@zmoog zmoog Sep 17, 2024

Choose a reason for hiding this comment

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

Hey Andrew, thanks for the heads up. This is a bug; updating the changelog classification accordingly.

@zmoog zmoog enabled auto-merge (squash) September 17, 2024 09:24
@zmoog zmoog merged commit 6d76da8 into elastic:main Sep 17, 2024
@elasticmachine
Copy link
Copy Markdown

elasticmachine commented Sep 17, 2024

💚 Build Succeeded

History

cc @zmoog

@elastic-sonarqube
Copy link
Copy Markdown

elastic-sonarqube bot commented Sep 17, 2024

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

harnish-crest-data pushed a commit to chavdaharnish/integrations that referenced this pull request Feb 4, 2025
@zmoog
graphactivitylogs: Fix client|source.geo.location mapping (elastic#11102
2c058f0 
)

Align `client|source.geo.location` fields to ECS mapping.

Users reported mapping exceptions due to Elasticsearch mapping the `client|source.geo.location` fields as `object` instead of `geo_point`. See elastic#10848 for more.
harnish-crest-data pushed a commit to chavdaharnish/integrations that referenced this pull request Feb 5, 2025
@zmoog
graphactivitylogs: Fix client|source.geo.location mapping (elastic#11102
94dab57 
)

Align `client|source.geo.location` fields to ECS mapping.

Users reported mapping exceptions due to Elasticsearch mapping the `client|source.geo.location` fields as `object` instead of `geo_point`. See elastic#10848 for more.
@zmoog zmoog deleted the zmoog/fix-azire-graphactivitylogs-geo-location branch February 6, 2025 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewkroh andrewkroh andrewkroh left review comments

@efd6 efd6 efd6 left review comments

@kcreddy kcreddy kcreddy approved these changes

@muthu-mps muthu-mps muthu-mps approved these changes

Assignees

@zmoog zmoog

Labels

bugfix Pull request that fixes a bug issue Integration:azure Azure Logs Team:Security-Service Integrations Security Service Integrations team [elastic/security-service-integrations]

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@zmoog @elasticmachine @andrewkroh @kcreddy @efd6 @muthu-mps