-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Observability] Make Alerts page use shared Kibana time range #115192
[Observability] Make Alerts page use shared Kibana time range #115192
Conversation
@elasticmachine merge upstream |
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
Functionality works well and as expected 👌 I'll take a look through the code now. One question: I see the |
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 👍
Hopefully this can serve as a blueprint to using the stateContainer
and stateStorage
elsewhere (we've talked about it for logs code, for sure).
timefilterService.setTime(dateRange); | ||
setRangeFrom(dateRange.from); | ||
setRangeTo(dateRange.to); | ||
setKuery(query); |
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.
Much cleaner 👍
|
||
type AlertsPageStateContainer = typeof alertsPageStateContainer; | ||
|
||
const { Provider, useContainer } = createStateContainerReactHelpers<AlertsPageStateContainer>(); |
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.
Nice 👍
(It's my first time reviewing the state container in a real world example, so it's nice to see there's some helpers and so on)
const timefilterService = useTimefilterService(); | ||
|
||
useEffect(() => { | ||
const urlStateStorage = createKbnUrlStateStorage({ history, useHash: false }); |
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.
Asked my question before seeing this 😅
So we shouldn't see the hash?
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 part is a bit confusing. The URL state storage helper offers to options: store the state plainly in the URL (Rison encoded) OR store it into sessionStorage but store the key of the session storage entry in the URL (the key is a hash).
This is still different from the sessionStorage helper which only stores the state into session storage but does not update the URL.
The # (hash) that gets added to the URL is added regardless which is causing issues in the tests and is a blocker right now. We need to decide how to work around that.
This message gives some hints on what might be required to fix that:
https://elastic.slack.com/archives/CFFQ7RP9B/p1634211896180000?thread_ts=1634207518.178700&cid=CFFQ7RP9B
So I see 3 paths:
- Don't adopt the state container right now and just solve the issue in the ticket with the timefilter service (and create a ticket to follow up on the state containers to support history state as well)
- Modify the FTR helpers to support this weird mix
- Modify the state container to support history state
...lugins/observability/public/pages/alerts/state_container/use_alerts_page_state_container.tsx
Show resolved
Hide resolved
...lugins/observability/public/pages/alerts/state_container/use_alerts_page_state_container.tsx
Show resolved
Hide resolved
@Kerry350 |
…lerts-timeframe-with-observability
@Dosant Can you have a look at the changes I made in src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.ts? I still need to update the tests for those changes, but pushing this to get feedback from my team on some other things :) |
@@ -191,6 +191,25 @@ describe('KbnUrlStateStorage', () => { | |||
}); | |||
}); | |||
|
|||
describe('useHashQuery: true', () => { |
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'm not sure which other tests need to be mirrored from the default false case, or if there are some other cases to test, please advice :) @Dosant
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.
If it isn't too much burden, let's mirror KbnUrlStateStorage > useHash: false
🙏
…lerts-timeframe-with-observability
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 a couple of small nitpicks on the tests, feel free to ignore if you want. You mentioned they felt a bit messy, but they look good, great job 👍
I'm still waiting for Kibana to build right now to test the functionality again ⏳
x-pack/test/observability_functional/apps/observability/alerts/shared_time_range.ts
Outdated
Show resolved
Hide resolved
x-pack/test/observability_functional/apps/observability/alerts/shared_time_range.ts
Outdated
Show resolved
Hide resolved
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.
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 tested, didn't find any issues without #
in the URL. thanks for this improvement 👍
@@ -191,6 +191,25 @@ describe('KbnUrlStateStorage', () => { | |||
}); | |||
}); | |||
|
|||
describe('useHashQuery: true', () => { |
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.
If it isn't too much burden, let's mirror KbnUrlStateStorage > useHash: false
🙏
...history.location, | ||
search: nextSearchParams.toString(), | ||
}); | ||
timefilterService.setTime(dateRange); |
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.
it is also possible to subscribe to state container changes and update global time there. This way timefilterService will stay in sync with state container even if dateRange is updated from other places
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.
Good to know, there are some business requirements around defaults on page load that prevents us from doing that in this case though!
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
💔 Backport failedThe backport operation could not be completed due to the following error: The backport PRs will be merged automatically after passing CI. To backport manually run: |
@miltonhultgren Backport failed for this PR. Could you have a look? |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
I think the idea is to not backport this since it's a new feature. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Backport @miltonhultgren's changes to add the `useHashQuery` param to `createKbnUrlStateStorage` Co-authored-by: Milton Hultgren <milton.hultgren@elastic.co>
…117568) * [Fleet] Fix agent logs not reading query from URL (#117286) * Don't use hash for agent logs url state * revert core app services change * Cherrypick changes from PR #115192 Backport @miltonhultgren's changes to add the `useHashQuery` param to `createKbnUrlStateStorage` Co-authored-by: Milton Hultgren <milton.hultgren@elastic.co> Co-authored-by: Mark Hopkin <mark.hopkin@elastic.co> Co-authored-by: Milton Hultgren <milton.hultgren@elastic.co>
Summary
Closes #111348
This PR does two things:
Note: I'm still working on adding new E2E tests to test the new URL state and the timefilter service syncing.
We can consider merging this before FF and I can add those tests in a follow up PR.
Checklist