-
Notifications
You must be signed in to change notification settings - Fork 474
[AWS] Add support for Inspector datastream and Remove duplicate ECS fields #4604
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
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
🚀 Benchmarks reportTo see the full report comment with |
🌐 Coverage report
|
@vinit-elastic Could we actually remove the duplicate fields from agent.yml and keep them in the ecs.yml instead? Thanks!! I just did this for the ec2 integration: #4567 |
@kaiyan-sheng the reason why I removed fields from ecs.yml is because the agent.yml is a static file and is the same across all the connectors. The same has been discussed with @P1llus earlier, and we came to a conclusion not to change it unless there's a type change in the newer ecs version. Therefore we didn't change it. |
@vinit-elastic I agree, if we want to change agent.yml we should simply do it in one go for all packages instead @kaiyan-sheng ? |
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.
Hi @vinit-elastic! I'm going to request that we use the new Lens metric vis instead of the TSVB metric on the dashboard since it's a good fit and we're trying to get away from TSVB. Here's a screenshot of how you can configure it in the Lens editor:
And a couple optional requests:
-
Could you post a screenshot of this dashboard with data in it?
-
This is a big dashboard (16 visualizations). The concern with these is that they can load really slowly if the customer has a lot of data. All visualizations (including tables) (re)render every time the user navigates to the dashboard/adds a filter/changes the time range, regardless of whether or not they are above the fold. Is there a logical way it could make sense to break this up into smaller dashboards connected by hyperlinks?
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.
Thanks for making the visualization type change!
@P1llus For all packages, do you mean all integrations in AWS? I'm thinking to do that since I just did it for the ec2 integration. I think we should use ECS fields (with ecs.yml) when they are already defined in ECS so we don't have to duplicate the definition in agent.yml. |
@vinit-elastic How about lets keep the duplicated ECS fields for now, and make a separate PR to remove all the duplicate fields from agent.yml instead? |
Hey @kaiyan-sheng, if we keep the duplicate fields as they are, then the elastic-package check command gives an error. because both files have different types of descriptions and there is a type change in the newer Stack version as well as the ECS version. The same has been discussed with @P1llus earlier and we came to a conclusion that keep agent.yml as it is and remove duplicate fields in the ECS file. |
@vinit-elastic OK! Thanks for the explanation. I will make a separate PR later after this one gets in to fix the agent.yml and ecs.yml then! |
I made the change for removing duplicate fields from agent.yml and use ecs.yml instead in PR: #4657 |
Hey @kaiyan-sheng - I saw your PR, appreciate your effort. |
@vinit-elastic @P1llus Sorry thats the part I don't quite get. Why we are redefining ECS fields in |
@kaiyan-sheng Its mostly because all the integrations have been setup like this as far as I know, the fields are simply copied from the other integrations. We could change it, but I rather keep it out of this PR and do it over all integrations if you want. There is plenty of ECS fields in agent.yml from before. |
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.
@P1llus and I had a conversation offline and decides to merge my PR for moving all fields into ecs.yml first to resolve this issue. Sorry @vinit-elastic you have to rebase your PR and sorry for blocking it for a while... The part for adding support for inspector data stream looks good to me. Thank you both!!
What does this PR do?
Checklist
changelog.yml
file.manifest.yml
file to point to the latest Elastic stack release (e.g.^8.4.0
).How to test this PR locally
Related issues
Screenshots