Skip to content

sei: remove duplicate fields #4327

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 29, 2022
Merged

sei: remove duplicate fields #4327

merged 3 commits into from
Sep 29, 2022

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented Sep 28, 2022 •
edited
Loading

What does this PR do?

This fixes a number of duplicated field definitions required to get CI builds to pass. There may be duplicates in other SEI packages that have not been identified. These should be addressed when they are moved to storage format v2.

There are cases where fields are defined in base-fields that could be defined with external reference to the ECS. These have not been changed.

In order to get the juniper_netscreen package to pass, the formatting of MAC addresses was fixed to conform to the ECS.

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

Screenshots

@efd6 efd6 added bug Something isn't working, use only for issues integration Label used for meta issues tracking each integration Team:Security-External Integrations labels Sep 28, 2022
@efd6 efd6 self-assigned this Sep 28, 2022
@efd6 efd6 force-pushed the duplicated_fields branch from d826dac to 4c59e6f Compare September 28, 2022 01:04
@elasticmachine
Copy link

elasticmachine commented Sep 28, 2022 •
edited
Loading

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@elasticmachine
Copy link

elasticmachine commented Sep 28, 2022 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-09-29T03:41:09.088+0000

  • Duration: 21 min 5 sec

Test stats 🧪

Test Results
Failed 0
Passed 756
Skipped 0
Total 756

🤖 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
Copy link

elasticmachine commented Sep 28, 2022 •
edited
Loading

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (28/28) 💚
Files 100.0% (103/103) 💚
Classes 100.0% (103/103) 💚
Methods 96.366% (663/688) ❕
Lines 88.126% (15882/18022) ❕
Conditionals 100.0% (0/0) 💚

@efd6 efd6 force-pushed the duplicated_fields branch 5 times, most recently from f217603 to 323d47a Compare September 28, 2022 07:23
@efd6 efd6 force-pushed the duplicated_fields branch from 323d47a to c33fcd9 Compare September 28, 2022 10:55
@efd6
Copy link
Contributor Author

efd6 commented Sep 28, 2022

/test

@efd6 efd6 force-pushed the duplicated_fields branch from c33fcd9 to 5ad9e27 Compare September 28, 2022 22:17
@efd6 efd6 marked this pull request as ready for review September 28, 2022 22:38
@efd6 efd6 requested a review from a team as a code owner September 28, 2022 22:38
@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

andrewkroh
andrewkroh reviewed Sep 29, 2022
View reviewed changes
packages/ti_cif3/data_stream/feed/fields/base-fields.yml
@@ -10,10 +10,6 @@
- name: '@timestamp'
type: date
description: Event timestamp.
- name: event.module
type: constant_keyword
description: Event module
Copy link
Member

Choose a reason for hiding this comment

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

This definition is important because it includes the constant_keyword value of ti_cif3. The other definition is not a constant_keyword so this could affect query performance. Also I'm guessing that nothing sets a value for event.module in the _source so without this the field will go away.

Copy link
Member

Choose a reason for hiding this comment

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

So in general we would want to prefer the non-ECS definition when it's for an override to use constant_keyword. But for everything else I think the preference goes toward retaining the ECS definition and remove the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Will go over them again and check for these and revert the ones that are constant_keyword.

@efd6
Copy link
Contributor Author

efd6 commented Sep 29, 2022

PTAL

andrewkroh
andrewkroh approved these changes Sep 29, 2022
View reviewed changes
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM

@efd6 efd6 merged commit bbf0e61 into elastic:main Sep 29, 2022
This was referenced Sep 29, 2022
Closed
Closed
@andrewkroh andrewkroh mentioned this pull request Sep 29, 2022
Merged
@efd6 efd6 deleted the duplicated_fields branch February 5, 2025 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewkroh andrewkroh andrewkroh approved these changes

Assignees

@efd6 efd6

Labels
bug Something isn't working, use only for issues integration Label used for meta issues tracking each integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@efd6 @elasticmachine @andrewkroh