feat: search in service logs #1830
Conversation
# Conflicts: # enclave-manager/web/src/components/enclaves/logs/LogViewer.tsx
# Conflicts: # engine/server/webapp/asset-manifest.json # engine/server/webapp/index.html # engine/server/webapp/static/js/main.045e0a73.js.LICENSE.txt # engine/server/webapp/static/js/main.045e0a73.js.map # engine/server/webapp/static/js/main.14d7f9cc.js.LICENSE.txt # engine/server/webapp/static/js/main.14d7f9cc.js.map # engine/server/webapp/static/js/main.9e07637a.js # engine/server/webapp/static/js/main.9e07637a.js.LICENSE.txt # engine/server/webapp/static/js/main.9e07637a.js.map
🤖 I have created a release *beep* *boop* --- ## [0.85.33](0.85.32...0.85.33) (2023-11-20) ### Features * search in service logs ([#1830](#1830)) ([7fce5b5](7fce5b5)), closes [#1791](#1791) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: kurtosisbot <[email protected]>
There was a problem hiding this comment.
Nice job getting this out - I expect a decent amount of the feedback I've left here you're already aware of. The main piece of feedback I have is a strong preference to avoid chains of useEffect in response to callbacks - they make code harder to read and can eventually introduce bugs that are hard to trace. React has some notes on how to identify and remove avoidable effects here https://react.dev/learn/you-might-not-need-an-effect
| export type LogLineInput = { | ||
| logLineProps: LogLineProps; | ||
| logLineSearch?: LogLineSearch; | ||
| selected: boolean | undefined; | ||
| }; | ||
|
|
||
| const logFontFamily = "Menlo, Monaco, Inconsolata, Consolas, Courier, monospace"; | ||
|
|
||
| export const LogLine = ({ timestamp, message, status }: LogLineProps) => { |
There was a problem hiding this comment.
Here LogLineProps should have just become:
export type LogLineProps = {
timestamp?: DateTime;
message?: string;
status?: LogStatus;
selected?: boolean;
logLineSearch?: LogLineSearch;
}
| const HighlightPattern = ({ | ||
| text, | ||
| regex, | ||
| selected, | ||
| }: { | ||
| text: string; | ||
| regex: RegExp; | ||
| selected: boolean | undefined; | ||
| }) => { |
There was a problem hiding this comment.
I don't think this component (because that's what it is) should be inlined with LogLineSearch - this means it's redefined on every render and defined for every LogLine. I think it's LogLine specific enough to just be another non exported component in this file.
| const processText = (message: string) => { | ||
| if (hasAnsi(message)) { | ||
| return parse(convert.toHtml(message)); | ||
| const processText = (text: string, selected: boolean | undefined) => { |
There was a problem hiding this comment.
this is also actually another react component - it should be moved out of the LogLine component and given a better name as a (non exported) component like Message
| @@ -4,6 +4,9 @@ import { DateTime } from "luxon"; | |||
| import { isDefined } from "../../../utils"; | |||
| // @ts-ignore | |||
There was a problem hiding this comment.
Just noticed this - lets not use has-ansi (or its dependency ansi-regex) at all and just inline the code in src/utils/index.ts (and combine it with stripAnsi):
export function ansiRegex({ onlyFirst = false } = {}) {
const pattern = [
"[\\u001B\\u009B][[\\]()#;?]*(?:(?:(?:(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]+)*|[a-zA-Z\\d]+(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]*)*)?\\u0007)",
"(?:(?:\\d{1,4}(?:;\\d{0,4})*)?[\\dA-PR-TZcf-nq-uy=><~]))",
].join("|");
return new RegExp(pattern, onlyFirst ? undefined : "g");
}
const regex = ansiRegex({ onlyFirst: true });
export function hasAnsi(text: string) {
return regex.test(text);
}
| @@ -4,6 +4,9 @@ import { DateTime } from "luxon"; | |||
| import { isDefined } from "../../../utils"; | |||
| // @ts-ignore | |||
| import hasAnsi from "has-ansi"; | |||
| import { ReactElement } from "react"; | |||
| import { normalizeLogText } from "./LogViewer"; | |||
There was a problem hiding this comment.
This is a circular dependency now between LogLine and LogViewer - this should be avoided, and this method should be moved to a utils file.
| const searchTerm = normalizeLogText(rawText); | ||
| const logLineSearch: LogLineSearch = { | ||
| searchTerm: searchTerm, | ||
| pattern: new RegExp(searchTerm, "gi"), // `i` is invariant case |
| if (!search) return false; | ||
| return isDefined(search.searchTerm) && isNotEmpty(search.searchTerm); |
There was a problem hiding this comment.
(nit):
return isDefined(search?.searchTerm) && isNotEmpty(search.searchTerm);
| debouncedUpdateSearchTermCallback(e.target.value); | ||
| }; | ||
|
|
||
| const priorMatch = () => { |
There was a problem hiding this comment.
(very minor) wasn't immediately obvious to me this is a function - should be named something like handlePriorMatchClick
| useEffect(() => { | ||
| if (virtuosoRef?.current && currentSearchIndex !== undefined && currentSearchIndex >= 0) { | ||
| virtuosoRef.current.scrollToIndex(searchMatchesIndices[currentSearchIndex]); | ||
| } | ||
| }, [currentSearchIndex]); |
There was a problem hiding this comment.
this should not be a useEffect and should probably be either in a callback or called direct from the navigation/find matches code.
| <Editable | ||
| p={0} | ||
| m={0} | ||
| size={"sm"} | ||
| value={`${currentSearchIndex + 1}`} | ||
| onChange={(inputString) => | ||
| updateSearchIndexBounded(parseMatchIndexRequest(inputString) - 1) | ||
| } | ||
| > |
There was a problem hiding this comment.
I'm not familiar with this component, but wondering if we can apply numeric constraint to it.
## Description: This PR includes a number or emui enhancements: * Artifacts view * fix to dict/list input sizes * Update logging ui to match designs * Update general app layout to match designs (headers) Additionally the feedback left on #1830 is implemented. ### Demo of the artifacts view https://github.com/kurtosis-tech/kurtosis/assets/4419574/0849e46e-c410-4a07-a65f-bcc74297c53b ## Is this change user facing? YES
Description:
Adding a search bar to the service logs window
Is this change user facing?
YES
References (if applicable):
Closes #1791