Explore: Clear live logs #64237

abdulhdr1 · 2023-03-06T15:10:07Z

What is this feature?

A button that allows the user to clear the incoming live tailing logs, viewing only the newcoming ones

Why do we need this feature?

Sometimes, when viewing live logs, it's useful to look only at new messages

Which issue(s) does this PR fix?:

ifrost · 2023-03-17T09:07:10Z

Thanks @abdulhdr1 for submitting this! @grafana/observability-logs could you please have a look at this? Tested it locally and works nicely 👍 I just wonder if that could be somehow added to useLiveTailControls 🤔

abdulhdr1 · 2023-03-18T01:28:24Z

Sure! It was my bad, I'm still getting familiar with the codebase 😅 . After coming to the problem again it's a better approach as the logic should be contained within redux, will work on moving it to useLiveTailControls

ivanahuckova

This is amazing! 🚀 Really great job with this! I think this is going to be a great feature for live tailing. I've left some feedback, let me know what do you think, if you have any questions.

public/app/types/explore.ts

ivanahuckova · 2023-03-28T12:48:44Z

public/app/features/explore/LiveLogs.tsx

@@ -147,6 +159,10 @@ class LiveLogs extends PureComponent<Props, State> {
          </tbody>
        </table>
        <div className={styles.logsRowsIndicator}>
+          <Button variant="secondary" onClick={onClear} className={styles.button}>


I would move this button after the Resume|Pause button as that is the most important one in row.

also you can use the icon prop on the button to have the icon properly rendered as a prefix (and without the need to add the "margin" with  )

Got it, moved Clear logs to be the second one and use the Button icon prop, I thought it was nice to make it consistent in close use cases, so I've also changed Resume | Pause to use the prop, but couldn't change the Exit live mode button because it uses the type Icon prop that isn't available on Button's prop API (maybe a new issue?)

I wouldn't mind if we used square-shape icon without the type mono (that makes square filled) , as we already use square-shape without mono in live tailing button on top. So it would actually make it more consistent.

ivanahuckova · 2023-03-28T12:51:50Z

public/app/features/explore/LiveLogs.tsx

    }
    return rowsToRender;
  };

+  orderRowsByTime = (rows: LogRowModel[]) => {


In public/app/features/logs/utils.ts we already have sortLogRows function. Could we use that instead?

Sure, I didn't notice that there was one already 😅

ivanahuckova · 2023-03-28T12:59:17Z

public/app/features/explore/__mocks__/makeLogs.ts

@@ -0,0 +1,31 @@
+import { MutableDataFrame, LogLevel, LogRowModel } from '@grafana/data';
+export const makeLogs = (numberOfLogsToCreate: number, overrides?: Partial<LogRowModel>): LogRowModel[] => {


ivanahuckova · 2023-03-28T13:08:35Z

public/app/features/explore/state/query.ts

@@ -1065,7 +1108,7 @@ export const processQueryResponse = (
    graphResult,
    tableResult,
    rawPrometheusResult,
-    logsResult,
+    logsResult: filterLogRows(state, logsResult),


nit pick - to make it more clear that we filter logs only if we are in live mode, I'd do something like logsResult: state.isLive ? filterLogRows(state.clearedAt, logsResult) : logsResult

Hmmm, thinking about it, couldn't we make filterLogRows -> filterLogRowsByTime accept log rows and clearedAt and then use it also in LiveLogs.ts?

ivanahuckova · 2023-03-30T09:13:29Z

@abdulhdr1 whenever this is ready for review, could you please re-request them. Also, it seems that there is a merge conflict so you need to merge main, before we can review again.

Elfo404 · 2023-03-31T05:38:46Z

given we are filtering the logs every time we get more data, i'm wondering what will be the performance implications 🤔
the fact that i can now clear logs means that (i suppose) a user will be less likely to leave the view (or refresh), as they can just clear the logs when they want to se only new data.

This means that potentially, given enough time, we'll have to filter a huge number on logs based on their timestamp.

What if instead clearing logs saved the size of the array and when rendering we only display allLogs.slice(state.lastIndex)? it should be way faster.
although i'm not sure whether we actually need to modify the state to make to avoid any side effect. wdyt @ivanahuckova ?

btw sorry for the delay, i'll take some time to review this properly on monday

ivanahuckova · 2023-03-31T09:13:32Z

@Elfo404 yeah, that is a very good point, performance-wise allLogs.slice(state.lastIndex) makes more sense. I like clearedAt from readability/possible reusability point, but it is true that with live tailing, you can have 1000s of log lines coming in. So maybe we could just change clearedAt -> clearedAtIndex and use the slice.

abdulhdr1 · 2023-03-31T14:27:01Z

given we are filtering the logs every time we get more data, i'm wondering what will be the performance implications

Yeah, I hadn't thought of it before but it's surely going to affect performance.
Changing clearedAt to clearedAtIndex, as @ivanahuckova suggested, seems to be the way, will work on making the changes

ivanahuckova

I have found 2 bugs:

If you clear logs and then press pause, no logs are visible anymore
We initially load logs from your previous query (if you've run some). In my case, I have line limit set to 1000, so initially I have 1000 logs. If I have slow-coming logs and if click on clear logs before the first batch of live tailing logs arrive, the clearAtIndex will be 999 and I won't see any incoming logs.

edge.case.live.tailing.mov

abdulhdr1 · 2023-04-04T04:20:36Z

If you clear logs and then press pause, no logs are visible anymore

Sorry, I don't get it, should the cleared logs "return" when you pause?

We initially load logs from your previous query (if you've run some). In my case, I have line limit set to 1000, so initially I have 1000 logs. If I have slow-coming logs and if click on clear logs before the first batch of live tailing logs arrive, the clearAtIndex will be 999 and I won't see any incoming logs.

I worked on this in the last commit, but I don't know if there's a better way of checking if it's the first batch. Also, I'm basing it on the assumption that the state can only go Loading -> Streaming on this scenario, is that fair to assume?

grafanabot · 2023-04-04T07:43:03Z

This pull request was removed from the 9.5.0 milestone because 9.5.0 is currently being released.

ivanahuckova · 2023-04-06T14:34:53Z

First of all thank you for being so responsive and working on this useful feature! Really appreciated!! 🧡 And I am excited to see it being part of Live tailing.

I worked on this in the last commit, but I don't know if there's a better way of checking if it's the first batch. Also, I'm basing it on the assumption that the state can only go Loading -> Streaming on this scenario, is that fair to assume?

I am wondering if you have considered adding this kind of logic to clearLogs, so we are handling everything related to clearLogs only in there and not in queryStreamUpdatedAction? In this case, we would be changing clearedAtIndex only if we have streaming state.

What do you think about this?

Sorry, I don't get it, should the cleared logs "return" when you pause?

Oh, could be an issue on my side. When I pressed pause, all logs in live tailing disappeared for me even if I didn't press clear logs. But I can't reproduce anymore, so feel free to disregard.

abdulhdr1 · 2023-04-12T13:09:58Z

The pleasure is all mine, thank you for the feedback on my code, I'm looking forward to contributing to other features!

What do you think about this?

That surely would be a better design, but I didn't want to make the clear logs button unresponsive during the loading state, maybe passing a disabled prop when state === "loading" should solve it.

ivanahuckova · 2023-04-13T13:32:55Z

I didn't want to make the clear logs button unresponsive during the loading state, maybe passing a disabled prop when state === "loading" should solve it.

Not sure if I understand. The loading state is only at the beginning when we display logs from previously run "normal" query (that was displayed in regular logs view) in live tailing view. So if we use clear action - that returns empty rows and set clearAtIndex to null, there shouldn't be any unresponsiveness.

And then when all the new logs come trough streaming, we can correctly set clearAtIndex to null.

But maybe I am missing something. 🤔

abdulhdr1 · 2023-04-14T01:29:12Z

You're right. I must have misunderstood something before, there is no unresponsiveness. I'm going to add the change you suggested

ivanahuckova

LGTM! 🚀 Really great job with this. @Elfo404 would you have a look as well.

public/app/features/explore/state/query.ts

Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com>

Elfo404

🚀

* feat: add clear logs to explorer's live logs * refactor live logs pause/resume updating logic * add tests for clearing live logs * rm unused imports * move `onClear` live logs logic to redux * move clear logs tests to `query.test` * use utils `sortLogsRows` and Button's icon props * rename `filterLogRows` and add `clearedAt` as an arg * mv clearedAt type closer to live tailing items and add jsdoc * mv `filterLogRowsByTime` to `/utils` and use it at `LiveLogs` component * make `Exit live` button consistent with other actions * use `sortLogRows` and fix timestamp by id on `makeLogs` * change clear live logs from based on timestamp to index on logRows * assign `null` to `clearedAtIndex` on first batch of logs live * move `isFirstStreaming` implementation to `clearLogs` reducer * fix `clearLogs` tests * Update public/app/features/explore/state/query.ts Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com> --------- Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com>

abdulhdr1 added 3 commits March 6, 2023 00:18

feat: add clear logs to explorer's live logs

b042f7c

refactor live logs pause/resume updating logic

533f359

add tests for clearing live logs

c84337c

abdulhdr1 requested a review from a team as a code owner March 6, 2023 15:10

grafanabot added area/explore area/frontend pr/external This PR is from external contributor labels Mar 6, 2023

abdulhdr1 changed the title ~~Feature/clear live logs~~ Explore: Clear live logs Mar 6, 2023

rm unused imports

aea1582

ifrost added this to the 9.5.0 milestone Mar 7, 2023

ifrost added add to changelog no-backport Skip backport of PR labels Mar 7, 2023

ifrost requested a review from a team March 7, 2023 08:51

ifrost removed the request for review from a team March 17, 2023 09:07

abdulhdr1 added 2 commits March 18, 2023 18:32

move onClear live logs logic to redux

f7a20ee

move clear logs tests to query.test

68458a5

abdulhdr1 requested a review from a team as a code owner March 19, 2023 20:06

abdulhdr1 requested review from ashharrison90 and JoaoSilvaGrafana and removed request for a team March 19, 2023 20:06

ivanahuckova reviewed Mar 28, 2023

View reviewed changes

Abdul haidar and others added 5 commits March 28, 2023 15:54

use utils sortLogsRows and Button's icon props

cabbae6

rename filterLogRows and add clearedAt as an arg

02089dc

mv clearedAt type closer to live tailing items and add jsdoc

3774f72

mv filterLogRowsByTime to /utils and use it at LiveLogs component

9e2d8f2

make Exit live button consistent with other actions

0fe1a3d

use sortLogRows and fix timestamp by id on makeLogs

7e23204

Merge branch 'main' into feature/clear-live-logs

393cd09

abdulhdr1 requested review from ivanahuckova and removed request for ashharrison90 and JoaoSilvaGrafana March 30, 2023 15:37

ivanahuckova requested a review from Elfo404 March 30, 2023 18:36

change clear live logs from based on timestamp to index on logRows

db02e92

ivanahuckova reviewed Apr 3, 2023

View reviewed changes

assign null to clearedAtIndex on first batch of logs live

c254335

grafanabot removed this from the 9.5.0 milestone Apr 4, 2023

abdulhdr1 added 2 commits April 13, 2023 22:29

move isFirstStreaming implementation to clearLogs reducer

44c6915

fix clearLogs tests

e3b4325

ivanahuckova approved these changes Apr 14, 2023

View reviewed changes

public/app/features/explore/state/query.ts Show resolved Hide resolved

Update public/app/features/explore/state/query.ts

802e3a2

Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com>

ivanahuckova added this to the 10.0.0 milestone Apr 18, 2023

Elfo404 approved these changes Apr 20, 2023

View reviewed changes

ivanahuckova merged commit 3a013cb into grafana:main Apr 20, 2023
12 checks passed

abdulhdr1 deleted the feature/clear-live-logs branch April 22, 2023 18:28

zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explore: Clear live logs #64237

Explore: Clear live logs #64237

abdulhdr1 commented Mar 6, 2023

ifrost commented Mar 17, 2023

abdulhdr1 commented Mar 18, 2023

ivanahuckova left a comment

ivanahuckova Mar 28, 2023

Elfo404 Mar 28, 2023

abdulhdr1 Mar 28, 2023

ivanahuckova Mar 29, 2023

ivanahuckova Mar 28, 2023

abdulhdr1 Mar 28, 2023 •

edited

ivanahuckova Mar 28, 2023

ivanahuckova Mar 28, 2023

ivanahuckova Mar 28, 2023

ivanahuckova commented Mar 30, 2023

Elfo404 commented Mar 31, 2023

ivanahuckova commented Mar 31, 2023

abdulhdr1 commented Mar 31, 2023

ivanahuckova left a comment

abdulhdr1 commented Apr 4, 2023 •

edited

grafanabot commented Apr 4, 2023

ivanahuckova commented Apr 6, 2023 •

edited

abdulhdr1 commented Apr 12, 2023

ivanahuckova commented Apr 13, 2023

abdulhdr1 commented Apr 14, 2023

ivanahuckova left a comment

Elfo404 left a comment

		@@ -0,0 +1,31 @@
		import { MutableDataFrame, LogLevel, LogRowModel } from '@grafana/data';
		export const makeLogs = (numberOfLogsToCreate: number, overrides?: Partial<LogRowModel>): LogRowModel[] => {

Explore: Clear live logs #64237

Explore: Clear live logs #64237

Conversation

abdulhdr1 commented Mar 6, 2023

ifrost commented Mar 17, 2023

abdulhdr1 commented Mar 18, 2023

ivanahuckova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdulhdr1 Mar 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanahuckova commented Mar 30, 2023

Elfo404 commented Mar 31, 2023

ivanahuckova commented Mar 31, 2023

abdulhdr1 commented Mar 31, 2023

ivanahuckova left a comment

Choose a reason for hiding this comment

abdulhdr1 commented Apr 4, 2023 • edited

grafanabot commented Apr 4, 2023

ivanahuckova commented Apr 6, 2023 • edited

abdulhdr1 commented Apr 12, 2023

ivanahuckova commented Apr 13, 2023

abdulhdr1 commented Apr 14, 2023

ivanahuckova left a comment

Choose a reason for hiding this comment

Elfo404 left a comment

Choose a reason for hiding this comment

abdulhdr1 Mar 28, 2023 •

edited

abdulhdr1 commented Apr 4, 2023 •

edited

ivanahuckova commented Apr 6, 2023 •

edited