-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Logs UI] Use the Super date picker in the log stream #54280
[Logs UI] Use the Super date picker in the log stream #54280
Conversation
b082ee9
to
f83254b
Compare
077aed4
to
2be7fbe
Compare
2be7fbe
to
b21a2e9
Compare
b21a2e9
to
02d8b59
Compare
c839bf2
to
6a89955
Compare
5856911
to
6dddc6b
Compare
💔 Build FailedTest FailuresKibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/infra/public/containers/logs/log_summary.useLogSummary hook queries for new summary buckets when the start and end date changesStandard Out
Stack Trace
Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/infra/public/components/logging/log_text_stream.LogEntryFieldColumn should output a
|
b9e1fde
to
4752771
Compare
x-pack/legacy/plugins/infra/common/http_api/log_entries/entries.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/common/http_api/log_entries/entries.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/common/http_api/log_entries/entries.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/common/http_api/log_entries/entries.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/common/http_api/log_entries/entries.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/public/containers/logs/log_entries/index.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/public/containers/logs/log_entries/index.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/log_highlights.tsx
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/infra/public/containers/logs/log_position/log_position_state.ts
Outdated
Show resolved
Hide resolved
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
4752771
to
a8c7581
Compare
I think it looks good in general. A few things that I found confusing:
|
One more question came to my mind: with this change, the |
The UI uses `null` extensively. It's easier to allow `null` as a value in the API than to refactor the UI to use `undefined` instead of `null`, so let's go with that.
The new API uses io-ts so we have removed the assertions that involve type checking. If these would be false the test will still fail when decoding the response.
@katrin-freihofner sorry for the late reply 🙈
AFAIK no :(
I think I'd go for no text instead of no icons, but maybe the UX becomes too bad? I think the buttons will need to be moved out eventually. There's an issue somewhere about using an existing searchbar+datepicker component that exists in kibana already, and if we use it we won't be able to add the buttons in between the search bar and the date selector, so this might be a good chance to think where they could go
|
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're almost there...
x-pack/plugins/infra/public/pages/logs/stream/page_providers.tsx
Outdated
Show resolved
Hide resolved
Instead of using `useMemo` to update the timestamps we will explicitly set them up when the user selects a new date range or, if the `endTimestamp` is `now`, when the user scrolls down. This still enables infinite scroll and live streaming, but avoids a lot of unnecessary fetching.
d0897d9
to
af41535
Compare
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 couldn't provoke any unnecessary refetches now. There's just one problem left with the streaming, it seems.
startLiveStreaming: useCallback(() => { | ||
setIsStreaming(true); | ||
jumpToTargetPosition(null); | ||
updateDateRange({ startDateExpression: 'now-1d', endDateExpression: '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.
Currently the live streaming doesn't fetch new entries.
I think the updateDateRange
has to happen on a an interval (useInterval
from react-use
, for example), doesn't it? It looks like streamEntriesEffect
in the log_entries/index.ts
drives the timestamp change right now, which sounds the wrong way around. Shouldn't updating the time range on an interval should drive the loading effect? Then that streamEntriesEffect
wouldn't have to await
the timeout anymore, but could just react to timestamp changes as long as the isStreaming
prop is 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.
So I tried this locally and now I understand why you were hesitant to make such changes 🙈
So now my question becomes: If we revert that latest change about the timestamp calculation, can we commit to immediately start cleaning this up so we don't release something that puts a large burden on the Elasticsearch cluster for a simple browser scroll?
If yes, then it might be prudent to roll back to the continuous endTimestamp
update and follow this up with a refactoring of the data flow. What do you think?
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 think the updateDateRange has to happen on a an interval
I'm not sure. updateDateRange
also refreshes the startTimestamp
, shifting the range and forcing a reload (...I think, because the effect that triggers a full reload has a isStreaming
condition somewhere).
It looks like streamEntriesEffect in the log_entries/index.ts drives the timestamp change right now, which sounds the wrong way around. Shouldn't updating the time range on an interval should drive the loading effect?
Yep, it's not obvious. The state for the streaming is handled in logPosition
but the behaviour is handled in logEntries
.
I wouldn't change it now. We can address this when we redo this data flow.
x-pack/plugins/infra/public/containers/logs/log_position/log_position_state.ts
Outdated
Show resolved
Hide resolved
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
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.
While I can't say that I feel totally confident into the data fetching logic (due to no fault of your work in this PR!), it seems to work pretty reliably and doesn't perform excessive refetches in my tests. I'm looking forward to the data flow cleanup. ✨
Thank you for sticking with it. ❤️ This was a very difficult change, but you mastered it. 👏
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.
Glad to see this come together!
Summary
Part of #49154
This PR replaces the point-in-time datepicker from the Logs UI with the
EuiSuperDatePicker
to allow users to specify a range.Changes
LogSummaryState
andLogEntriesState
now keep track of the date range. We represent the range with two pairs of values:startDateExpression
,endDateExpression
: the datemath expression (i.e.now-10m
,now
)startTimestamp
,endTimestamp
: the resolved datemath expression, in milliseconds.The
endTimestamp
is dynamic whenendDate
isnow
. This allows the streaming to always fetch the most recent entries.The minimap now has a fixed range, which corresponds to the
startDate
andendDate
. The minimap is no longer drag-able anymore either.LogSummaryState
has now two elements to consider: the date range and the log position cursor. They can be set with different callbacks and each change might trigger a refetching of entries. That means that if the user has both values (start and end dates, and a position) set in the initial URL, the page will fetch twice. To prevent this, we have added a flag to theLogSummaryState
that determines if it has been initialized or not. If it's not initialized, consumers of the API should not do anything.The GraphQL implementation of the API has been removed.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.(https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
For maintainers