- Notifications
You must be signed in to change notification settings - Fork 513
Mapping of Gigamon Metadata Attributes to ECS fields #14692
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
Conversation
31da13b to 9adc056 Compare | Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
| /test |
efd6 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the PR template. Can you provide a proposed commit message to repair that?
| type: keyword | ||
| - name: http_rtt | ||
| type: keyword | ||
| type: float |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not OK. Please revert this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a recent use case from customer, we need to perform math operation for this attribute. So, changed the data type from keyword to float
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this is a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apps-elastic-gigamon, any datatype changes not within the same family, for example: keyword family, is a breaking-change because it leads to mapping conflicts for existing users.
If this is must, you will need to update changelog version as 2.0.0 with type: breaking-change. Example:
integrations/packages/ti_abusech/changelog.yml
Lines 11 to 17 in ed4eea9
| - version: "3.0.0" | |
| changes: | |
| - description: | | |
| Auth Key (API Key) is now required for all AbuseCH API requests. Requests without authentication will be denied. | |
| See the official announcement: https://abuse.ch/blog/community-first/ | |
| type: breaking-change | |
| link: https://github.com/elastic/integrations/pull/14335 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes done as per the comments.
- version: "2.0.0"
changes:- description: Update mapping of Gigamon attributes to align with ECS fields
type: breaking-change
link: Mapping of Gigamon Metadata Attributes to ECS fields #14692
- description: Update mapping of Gigamon attributes to align with ECS fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than mutating existing tests, please add new tests that cover the additions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added new JSON records as per the comments
packages/gigamon/data_stream/ami/elasticsearch/ingest_pipeline/default.yml Outdated Show resolved Hide resolved
packages/gigamon/data_stream/ami/elasticsearch/ingest_pipeline/default.yml Outdated Show resolved Hide resolved
packages/gigamon/changelog.yml Outdated
| # newer versions go on top | ||
| - version: "1.7.0" | ||
| changes: | ||
| - description: Mapping of Gigamon Attributes to ECS fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a sentence; suggest "Improve mapping of Gigamon attributes to ECS fields."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are currently mapping the Gigamon attributes with the appropriate ECS fields, rather than making enhancements to the existing mappings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current text is not a valid English sentence since it has no verb. Please apply the change or something equivalent. The suggested change adds an appropriate verb since this is an improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the description as per comments
| type: long | ||
| - name: app_id | ||
| type: long | ||
| type: keyword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unfortunate, but cannot be changed without a breaking change. Please revert this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have mapped service.id ECS field to this app_id. Hence the data type has to be changed. Also, in the backend we haven't done any math operations using this app_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's unfortunate that the field was originally defined as a long rather than a keyword as is conventional for an ID field, but changing this from a long to a keyword is a breaking change, so making such a change would require a significant benefit to counter the cost of breaking people.
It does need to be a keyword when it is copied to event.id, but that can either be done as a convert to a string, or just by a copy since numeric values can be mapped as keywords.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the changelog.yml accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still do not believe this change is justified.
kcreddy left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is failing on error: Error: checking package failed: checking readme files are up-to-date failed: files do not match
To fix this CI error, you will need to run elastic-package build and commit the generated docs/README.md.
77c9706 to e65845c Compare | /test |
packages/gigamon/data_stream/ami/elasticsearch/ingest_pipeline/default.yml Show resolved Hide resolved
🚀 Benchmarks reportPackage |
| Data stream | Previous EPS | New EPS | Diff (%) | Result |
|---|---|---|---|---|
ami | 4694.84 | 2890.17 | -1804.67 (-38.44%) | 💔 |
To see the full report comment with /test benchmark fullreport
e65845c to 349ef32 Compare | /test |
kcreddy left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, to be merged after @efd6 comments are addressed.
| - version: "2.0.0" | ||
| changes: | ||
| - description: Update mapping of Gigamon attributes to align with ECS fields | ||
| type: breaking-change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not believe the balance of benefits/costs supports a breaking change here.
| type: long | ||
| - name: app_id | ||
| type: long | ||
| type: keyword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still do not believe this change is justified.
efd6 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reluctantly
| /test |
💚 Build Succeeded
History
|
|
| Package gigamon - 2.0.0 containing this change is available at https://epr.elastic.co/package/gigamon/2.0.0/ |
…#14692) This is an enhancement change for the mapping of Gigamon Metadata Attributes to corresponding ECS fields.




Proposed commit message :
This is an enhancement change for the mapping of Gigamon Metadata Attributes to corresponding ECS fields
Checklist
changelog.ymlfile.Related issues