Skip to content

Feat/console virtualization#224

Merged
johann-crabnebula merged 32 commits intomainfrom
feat/console-virtualization
Mar 5, 2024
Merged

Feat/console virtualization#224
johann-crabnebula merged 32 commits intomainfrom
feat/console-virtualization

Conversation

@johann-crabnebula
Copy link
Copy Markdown
Contributor

@johann-crabnebula johann-crabnebula commented Feb 7, 2024

TL;DR

This PR aims to improve the performance of the console tab. The goal was to at least be able to confidently display 10k logs and make the experience as smooth as possible. The dependency tanstack-virtual for solidjs was added to make sure we only render the DOM elements which are necessary for the view.

Acceptance criteria

  • General handling of 10k + logs
  • Switching between tabs without breaking (for example from console -> sources -> console / note that the calls tab is currently not working with a large number of spans, so please try to test the other tabs when switching, PR for the calls tab is coming)
  • Filtering should be smooth with logs coming in and when inactive
  • Console tab should work fine with cached logs when disconnected

Let me know

If there is anything odd in general please let me know.

Help with testing this

In my testing I just spammed events from my tauri app. Please feel free to be more creative with testing:

let mut count = 0;
// Infinite loop
loop {
    count += 1;
    tokio::time::sleep(Duration::from_millis(1)).await;
    window
        .emit(
            format!("test{}-event", count).as_str(),
            &EventPayload {
                key: "count",
                value: count.to_string(),
            },      
    ).unwrap();
    tracing::debug!("test{}-event", count);

    if count == 15000 {
        // Exit this loop
        break;
    }
}

Fixes DT-112

@netlify
Copy link
Copy Markdown

netlify Bot commented Feb 7, 2024

Deploy Preview for cn-devtools-app ready!

Name Link
🔨 Latest commit a2df0f5
🔍 Latest deploy log https://app.netlify.com/sites/cn-devtools-app/deploys/65e5fb6ac8bde1000810f653
😎 Deploy Preview https://deploy-preview-224--cn-devtools-app.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@johann-crabnebula johann-crabnebula force-pushed the feat/console-virtualization branch from c9b8e6a to 54c559a Compare February 7, 2024 10:22
@johann-crabnebula johann-crabnebula force-pushed the feat/console-virtualization branch from e0bedd9 to 529361e Compare February 8, 2024 03:55
@CrabNejonas
Copy link
Copy Markdown
Contributor

quick tip @johann-crabnebula in Rust you can do this to loop through a range of number:

    for i in 0..15000 {
        tokio::time::sleep(Duration::from_millis(1)).await;
        window
            .emit(
                format!("test{}-event", count).as_str(),
                &EventPayload {
                    key: "count",
                    value: count.to_string(),
                },
            )
            .unwrap();
        tracing::debug!("test{}-event", count);
    }

CrabNejonas
CrabNejonas previously approved these changes Feb 8, 2024
Copy link
Copy Markdown
Contributor

@CrabNejonas CrabNejonas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, not gonna argue with you about any JS stuff, but perfand usability is good 👍

CrabNejonas
CrabNejonas previously approved these changes Feb 8, 2024
Copy link
Copy Markdown
Contributor

@denjell-crabnebula denjell-crabnebula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Johann and I talked this through, and it seems good enough for now. I think we should ship it and then focus on speed / perf enhancements for the Tauri app version.

Copy link
Copy Markdown
Contributor

@atila-crabnebula atila-crabnebula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few questions, so I'll wait for Johann to be back before merging this.

Comment thread clients/web/src/components/console/log-event-entry.tsx
Comment on lines +65 to +68
<Show when={metadata?.location?.file}>
{(filePath) => (
<>
{getFileNameFromPath(filePath())}:{metadata?.location?.line ?? ""}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be better to handle the non-existing line within getFileNameFromPath and omitting the DOM node completely instead of printing an empty string. So the method returns the line or a falsy value (null, perhaps).

So the signature looks like:

<Show when={getFileNameFromPath(metadata?.location?.file)>
  {line => (<span>{line()</span>)}
</Show>

wdyt?

Copy link
Copy Markdown
Contributor Author

@johann-crabnebula johann-crabnebula Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix this, taking your suggestion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrote a getFileLineFunction that either returns a fully fledged line or null:

import { Location } from "../proto/common";
import { getFileNameFromPath } from "./get-file-name-from-path";

export function getFileLineFromLocation(location: Location | undefined) {
  if (!location || !location.file) return null;

  let line = getFileNameFromPath(location.file);

  if (location.line) line += `:${location.line}`;

  return line;
}

Template now looks like this:

<Show when={getFileLineFromLocation(metadata?.location)}>
  {(line) => <span>{line()}</span>}
</Show>

)
);
setMonitorData("spans", (spans) => [
...updatedSpans(spans, spansUpdate.spanEvents),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is mutating the entire array. Every time this setMonitorData is called, we replace the entire array. How about wrapping it in a reconcile() so only the needed keys are mutated? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to use Solid's reconcile here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the use of reconcile the updatedSpans function and the receiving piece on the calls tab might need updates. I would like to postpone this change to the calls virtualization PR.

Comment thread clients/web/src/views/dashboard/console.tsx
…zation

# Conflicts:
#	clients/web/package.json
#	clients/web/pnpm-lock.yaml
#	clients/web/src/components/autoscroll-pane.tsx
#	clients/web/src/lib/connection/transport.tsx
#	clients/web/src/lib/console/filter-logs.ts
#	clients/web/src/views/dashboard/console.tsx
@atila-crabnebula atila-crabnebula self-requested a review February 21, 2024 13:13
Copy link
Copy Markdown
Contributor

@atila-crabnebula atila-crabnebula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something is going on at the spans tab.

Bug

  1. Run tauri-v1 example
  2. Open the spans tab
  3. Fire a span test

It will never catch the end event.

@johann-crabnebula
Copy link
Copy Markdown
Contributor Author

Reverted the change for the spans. It will slow down the overall performance but this unblocks the PR, so we can move on.

@johann-crabnebula johann-crabnebula merged commit 068f98f into main Mar 5, 2024
@johann-crabnebula johann-crabnebula deleted the feat/console-virtualization branch March 5, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants