-
Notifications
You must be signed in to change notification settings - Fork 11
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
Feat/console virtualization #224
Conversation
✅ Deploy Preview for cn-devtools-app ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
c9b8e6a
to
54c559a
Compare
… running when it is set to default
…erformance at 5k + entries / move number of logs to the left of the filter
e0bedd9
to
529361e
Compare
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);
} |
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.
Looks good, not gonna argue with you about any JS stuff, but perfand usability is good 👍
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.
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.
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 have a few questions, so I'll wait for Johann to be back before merging this.
<Show when={metadata?.location?.file}> | ||
{(filePath) => ( | ||
<> | ||
{getFileNameFromPath(filePath())}:{metadata?.location?.line ?? ""} |
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 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?
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 will fix this, taking your suggestion.
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.
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), |
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 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? 🤔
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 will try to use Solid's reconcile
here
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.
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.
…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
3bf6a40
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.
something is going on at the spans tab.
Bug
- Run tauri-v1 example
- Open the spans tab
- Fire a span test
It will never catch the end event.
Reverted the change for the spans. It will slow down the overall performance but this unblocks the PR, so we can move on. |
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
forsolidjs
was added to make sure we only render the DOM elements which are necessary for the view.Acceptance criteria
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:
Fixes DT-112