-
Notifications
You must be signed in to change notification settings - Fork 422
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
[AWS] Add support for Guardduty datastream #4915
[AWS] Add support for Guardduty datastream #4915
Conversation
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
🌐 Coverage report
|
packages/aws/data_stream/guardduty/agent/stream/httpjson.yml.hbs
Outdated
Show resolved
Hide resolved
packages/aws/data_stream/guardduty/elasticsearch/ingest_pipeline/default.yml
Show resolved
Hide resolved
packages/aws/data_stream/guardduty/elasticsearch/ingest_pipeline/default.yml
Show resolved
Hide resolved
…into package-aws-guardduty-1.29.0
@efd6 is this ok to merge or any further changes required from Crest? |
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.
Dashboards are following the key best practices 👍
I see that you have used a non-Lens visualization: tag cloud. We will be adding tag clouds to Lens at some point but until then, we suggest using a Lens horizontal bar chart to convey the same information instead.
Of course, you can keep the legacy tag cloud here if you feel strongly. Just be aware that if you add non-Lens visualizations now, you will likely have to change them to Lens visualizations to use this dashboard on serverless.
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
Hey @drewdaemon, Thanks for the suggestion. The reason we opted for the tag cloud is that there could be multiple Threat Names and plotting it over a bar chart would result in clutter. If there are only a few values for threat names, I'd make sense to go for a bar chart in case of more than 10-15 names it would only show the first few names and others to be grouped under the "others" bucket and not grouping extra value would result in a visual mess. Let me know what are your thoughts. :) |
@vinit-elastic
Yes, I understand your dilemma. My first question would be: how many threat names is it actually useful to show? Is it important to show more than the top 10-20 values? Generally, there's a law of diminishing returns at play in data visualization. Horizontal bar chartThe horizontal bar chart actually can support quite a few. As a comparable, here's an unrelated chart of about the same dimensions displaying 25 names (not necessarily the suggested number, just picking something big to illustrate). It doesn't seem cluttered to me. The labels are readable, the bars are neatly aligned, and the user can mouse over any of them for more information. There is no "other bucket;" terms below the top 25 aren't shown. A bar chart also makes the difference between one term and another much more obvious than does a tag cloud. Tag cloudFor example, here's a tag cloud of the same data. Looking just at this tag cloud, I challenge you to tell me which is the more common term between That said, I understand the visual appeal of the tag cloud. They border on info-graphics. People like them for sure. Unfortunately, we're currently making you choose between using Lens and using a tag cloud. Ultimately, it's your job to do what's best for your users. |
Hey @drewdaemon - Appreciate the explanation. It makes a whole lot of sense now. |
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
Package aws - 1.31.0 containing this change is available at https://epr.elastic.co/search?package=aws |
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