[Observability RAC] Improve alerts table columns #105446
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mgiota
commented
Jul 14, 2021
| @@ -34,6 +38,28 @@ interface AlertsTableTGridProps { | |||
| setRefetch: (ref: () => void) => void; | |||
| } | |||
|
|
|||
| // TODO import EventsThContent from timelines plugin and customize a few css properties (position & padding-top) | |||
| const EventsThContent = styled.div.attrs(({ className = '' }) => ({ | |||
There was a problem hiding this comment.
@XavierM Requirement here was to add an Actions header, so temporarily I copied the styled EventsThContent component from the styles file of the t_grid component. I was thinking to import the component instead of copying, but I was wondering if there is another way to do custom styling on elements.
There was a problem hiding this comment.
The EventsThContent styled component may be deleted when the transition to EuiDataGrid is complete, so it's reasonable to copy it here, as opposed to making it part of the public API of tgrid (so it can be imported).
I was wondering if there is another way to do custom styling on elements.
The EuiDataGridControlColumn interface exposes headerCellRender to enable custom styling of header cells. This PR is correctly using it here:
headerCellRender: () => {
return <EventsThContent>Actions</EventsThContent>; // TODO: internationalization
},
to provide the custom styled header. I'm not aware of another way to do it, and this looks good to me. 😄
mgiota
commented
Jul 14, 2021
| @@ -203,7 +203,7 @@ export const EventsTbody = styled.div.attrs(({ className = '' }) => ({ | |||
| className: `siemEventsTable__tbody ${className}`, | |||
| role: 'rowgroup', | |||
| }))` | |||
| overflow-x: hidden; | |||
| overflow-x: scroll; | |||
There was a problem hiding this comment.
@XavierM Table is not responsive on small screen resolutions, but I guess this will be covered once moving to EUIDataGrid. So as a temporary solution I made the change directly to the timelines plugin to fix responsiveness.
Is anybody using the t-grid component of timelines plugin at the moment?
There was a problem hiding this comment.
I'm not sure how this change might impact the current implementation without some thorough desk testing, so my inclination is to hold off on this change for now, and only revisit it if the transition to EuiDataGrid is temporarily delayed.
Is anybody using the t-grid component of timelines plugin at the moment?
In addition to the o11y alerts page, the security solution is also using the t-grid component on several pages, behind a feature flag.
There was a problem hiding this comment.
@andrew-goldstein Ok I removed the horizontal scroll for now. Yep let's revisit responsiveness in case EuiDataGrid is delayed
|
@jasonrhodes I need input on this:
I turned off the extended formatter. Let me know if the default formatter is good enough:
@andrew-goldstein's comment
|
mgiota
force-pushed
the
105227-rac-alerts-table-columns
branch
from
6d8a556 to
d08c6eb
Compare
mgiota
added
the
Team:Infra Monitoring UI - DEPRECATED
|
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
|
@elasticmachine merge upstream |
mgiota
added
Theme: rac
x-pack/plugins/observability/public/pages/alerts/render_cell_value.tsx
Outdated
Show resolved
Hide resolved
andrew-goldstein
approved these changes
Jul 19, 2021
There was a problem hiding this comment.
Thanks for these improvements @mgiota! 🙏
Desk-tested locally
LGTM
weltenwort
approved these changes
Jul 19, 2021
x-pack/plugins/observability/public/pages/alerts/render_cell_value.tsx
Outdated
Show resolved
Hide resolved
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Firefox UI Functional Tests.test/functional/apps/discover/_field_data·ts.discover app discover tab field data the search term should be highlighted in the field dataStandard OutStack TraceMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @mgiota |
kibanamachine
added a commit
to kibanamachine/kibana
that referenced
this pull request
Jul 20, 2021
* right align duration on alerts observability table * reason column takes up the remaining width * add horizontal scrollbar to the table * add actions label temp solution * use abbreviated format for duration * Internationalization for actions * remove horizontal scroll and bring back initial width * remove unused import * remove data as dependency Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
kibanamachine
added a commit
that referenced
this pull request
Jul 20, 2021
* right align duration on alerts observability table * reason column takes up the remaining width * add horizontal scrollbar to the table * add actions label temp solution * use abbreviated format for duration * Internationalization for actions * remove horizontal scroll and bring back initial width * remove unused import * remove data as dependency Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: mgiota <[email protected]>
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Jul 20, 2021
…y-show-migrate-to-authzd-users * 'master' of github.com:elastic/kibana: (187 commits) Space management page UX improvements (elastic#100448) [Reporting] Unskip flaky test when downloading CSV with "no data" (elastic#105252) Update dependency @elastic/charts to v33 (master) (elastic#105633) [Observability RAC] Improve alerts table columns (elastic#105446) Introduce `preboot` lifecycle stage (elastic#103636) [Security Solution] Invalid kql query timeline refresh bug (elastic#105525) skip flaky suite (elastic#106121) [Security Solution][Endpoint] Fix UI inconsistency between isolation forms and remove display of Pending isolation statuses (elastic#106118) docs: APM RUM Source map API (elastic#105332) [CTI] Adds indicator match rule improvements (elastic#97310) [Security Solution] update text for Isolation action submissions (elastic#105956) EP Meta Telemetry Perf (elastic#104396) [Metrics UI] Drop partial buckets from ALL Metrics UI queries (elastic#104784) Remove beta admonitions for Fleet docs (elastic#106010) [Observability RAC] Remove indexing of rule evaluation documents (elastic#104970) Parameterize migration test for kibana version (elastic#105417) [Alerting] Allow rule to execute if the value is 0 and that mets the condition (elastic#105626) [ML] Fix Index data visualizer sometimes shows wrong doc count for saved searches (elastic#106007) [Security Solution] UX fixes for Policy page and Case Host Isolation comment (elastic#106027) [Security Solution]Memory protection configuration card for policies integration. (elastic#101365) ... # Conflicts: # x-pack/plugins/reporting/public/management/report_listing.test.tsx # x-pack/plugins/reporting/public/management/report_listing.tsx
andrew-goldstein
added a commit
to andrew-goldstein/kibana
that referenced
this pull request
Jul 26, 2021
This PR updates styles in the Observability `Alerts` table, as a follow-up to the [TGrid migrating to use `EuiDataGrid` for rendering](elastic#106199), and [this PR](elastic#105446), which improved the alerts table columns. - The `Reason` column uses up the remaining width, a follow-up task from <elastic#105446> - This task was originally tracked by <elastic#105227> - Increased the font weight and vertically aligned the `Actions` header with the other columns - Removed the `Status` column - Increased the width of the `Triggered` column - Renamed the `Duration` column to `Alert duration` - Eliminated the gap between actions ### Before  ### After  ### Desk testing - To desk test the `Observability > Alerts` page, add the following settings to `config/kibana.dev.yml`: ``` xpack.observability.unsafe.cases.enabled: true xpack.observability.unsafe.alertingExperience.enabled: true xpack.ruleRegistry.write.enabled: true ```
andrew-goldstein
added a commit
that referenced
this pull request
Jul 27, 2021
…06349) ## [Observability RAC] Alerts table post-`EuiDataGrid` style updates This PR updates styles in the Observability `Alerts` table, as a follow-up to the [TGrid migrating to use `EuiDataGrid` for rendering](#106199), and [this PR](#105446), which improved the alerts table columns. - The `Reason` column uses up the remaining width, a follow-up task from #105446 - This task was originally tracked by #105227 - Increased the font weight and vertically aligned the `Actions` header with the other columns - ~Removed the `Status` column~ (EDIT: we won't remove this, per a discussion w/ UX) - Increased the width of the `Triggered` column - ~Renamed the `Duration` column to `Alert duration`~ (EDIT: we won't rename this, per a discussion w/ UX) - Eliminated the gap between actions - Added truncation to the `Reason` column ### Before  ### After <img width="1280" alt="after" src="https://user-images.githubusercontent.com/4459398/126716690-be310fdf-3760-4014-998b-3c89099c2564.png"> ### Desk testing - To desk test the `Observability > Alerts` page, add the following settings to `config/kibana.dev.yml`: ``` xpack.observability.unsafe.cases.enabled: true xpack.observability.unsafe.alertingExperience.enabled: true xpack.ruleRegistry.write.enabled: true ``` cc @mdefazio
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Jul 27, 2021
…astic#106349) ## [Observability RAC] Alerts table post-`EuiDataGrid` style updates This PR updates styles in the Observability `Alerts` table, as a follow-up to the [TGrid migrating to use `EuiDataGrid` for rendering](elastic#106199), and [this PR](elastic#105446), which improved the alerts table columns. - The `Reason` column uses up the remaining width, a follow-up task from elastic#105446 - This task was originally tracked by elastic#105227 - Increased the font weight and vertically aligned the `Actions` header with the other columns - ~Removed the `Status` column~ (EDIT: we won't remove this, per a discussion w/ UX) - Increased the width of the `Triggered` column - ~Renamed the `Duration` column to `Alert duration`~ (EDIT: we won't rename this, per a discussion w/ UX) - Eliminated the gap between actions - Added truncation to the `Reason` column ### Before  ### After <img width="1280" alt="after" src="https://user-images.githubusercontent.com/4459398/126716690-be310fdf-3760-4014-998b-3c89099c2564.png"> ### Desk testing - To desk test the `Observability > Alerts` page, add the following settings to `config/kibana.dev.yml`: ``` xpack.observability.unsafe.cases.enabled: true xpack.observability.unsafe.alertingExperience.enabled: true xpack.ruleRegistry.write.enabled: true ``` cc @mdefazio
kibanamachine
added a commit
that referenced
this pull request
Jul 27, 2021
…06349) (#106923) ## [Observability RAC] Alerts table post-`EuiDataGrid` style updates This PR updates styles in the Observability `Alerts` table, as a follow-up to the [TGrid migrating to use `EuiDataGrid` for rendering](#106199), and [this PR](#105446), which improved the alerts table columns. - The `Reason` column uses up the remaining width, a follow-up task from #105446 - This task was originally tracked by #105227 - Increased the font weight and vertically aligned the `Actions` header with the other columns - ~Removed the `Status` column~ (EDIT: we won't remove this, per a discussion w/ UX) - Increased the width of the `Triggered` column - ~Renamed the `Duration` column to `Alert duration`~ (EDIT: we won't rename this, per a discussion w/ UX) - Eliminated the gap between actions - Added truncation to the `Reason` column ### Before  ### After <img width="1280" alt="after" src="https://user-images.githubusercontent.com/4459398/126716690-be310fdf-3760-4014-998b-3c89099c2564.png"> ### Desk testing - To desk test the `Observability > Alerts` page, add the following settings to `config/kibana.dev.yml`: ``` xpack.observability.unsafe.cases.enabled: true xpack.observability.unsafe.alertingExperience.enabled: true xpack.ruleRegistry.write.enabled: true ``` cc @mdefazio Co-authored-by: Andrew Goldstein <[email protected]>
streamich
pushed a commit
to vadimkibana/kibana
that referenced
this pull request
Aug 8, 2021
…astic#106349) ## [Observability RAC] Alerts table post-`EuiDataGrid` style updates This PR updates styles in the Observability `Alerts` table, as a follow-up to the [TGrid migrating to use `EuiDataGrid` for rendering](elastic#106199), and [this PR](elastic#105446), which improved the alerts table columns. - The `Reason` column uses up the remaining width, a follow-up task from elastic#105446 - This task was originally tracked by elastic#105227 - Increased the font weight and vertically aligned the `Actions` header with the other columns - ~Removed the `Status` column~ (EDIT: we won't remove this, per a discussion w/ UX) - Increased the width of the `Triggered` column - ~Renamed the `Duration` column to `Alert duration`~ (EDIT: we won't rename this, per a discussion w/ UX) - Eliminated the gap between actions - Added truncation to the `Reason` column ### Before  ### After <img width="1280" alt="after" src="https://user-images.githubusercontent.com/4459398/126716690-be310fdf-3760-4014-998b-3c89099c2564.png"> ### Desk testing - To desk test the `Observability > Alerts` page, add the following settings to `config/kibana.dev.yml`: ``` xpack.observability.unsafe.cases.enabled: true xpack.observability.unsafe.alertingExperience.enabled: true xpack.ruleRegistry.write.enabled: true ``` cc @mdefazio
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #105227
Here's how it looks with the fixes:
Current PR doesn't fix:
This is because current T-grid implementation isn't responsive and the
reason columngets cut off in small screen resolutions. If we make reason column use the remaining width, the column is cut off even in bigger screen resolutions. There is a temporary fix for this, but we will hold off for now.How to test
In order to get alerts in the
Observability > Alertspage, you should add following configuration:Then go into APM, set up some rules with sensitive thresholds, and you should be able to get some alerts. Setting up a log threshold rule will also produce some alerts.