Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: search in service logs #1830

Merged
merged 9 commits into from
Nov 20, 2023
Merged

feat: search in service logs #1830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3125223
wip
Nov 17, 2023
ac0be3b
Merge branch 'main' into anders/search
Nov 17, 2023
da2dffd
feat: search
Nov 17, 2023
a505e9f
adding highlighting
Nov 18, 2023
ba19516
Merge branch 'main' into anders/search
Nov 18, 2023
01b4afc
build
Nov 18, 2023
4304584
Merge branch 'main' into anders/search
Nov 20, 2023
58793f8
new build
Nov 20, 2023
d2b83a4
Merge branch 'main' into anders/search
adschwartz Nov 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 65 additions & 7 deletions enclave-manager/web/src/components/enclaves/logs/LogLine.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import { DateTime } from "luxon";
import { isDefined } from "../../../utils";
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
}

import hasAnsi from "has-ansi";
import { ReactElement } from "react";
import { normalizeLogText } from "./LogViewer";
Copy link
Contributor

Choose a reason for hiding this comment

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

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 Convert = require("ansi-to-html");
const convert = new Convert();

Expand All @@ -15,9 +18,21 @@ export type LogLineProps = {
status?: LogStatus;
};

export type LogLineSearch = {
searchTerm: string;
pattern: RegExp;
};

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) => {
Comment on lines +26 to -20
Copy link
Contributor

Choose a reason for hiding this comment

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

Here LogLineProps should have just become:

export type LogLineProps = {
      timestamp?: DateTime;
      message?: string;
      status?: LogStatus;
      selected?: boolean;
      logLineSearch?: LogLineSearch;
}

export const LogLine = ({ logLineProps, logLineSearch, selected }: LogLineInput) => {
const { timestamp, message, status } = logLineProps;
const statusToColor = (status?: LogStatus) => {
switch (status) {
case "error":
Expand All @@ -29,16 +44,58 @@ export const LogLine = ({ timestamp, message, status }: LogLineProps) => {
}
};

const processText = (message: string) => {
if (hasAnsi(message)) {
return parse(convert.toHtml(message));
const processText = (text: string, selected: boolean | undefined) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

let reactComponent;
if (hasAnsi(text)) {
reactComponent = parse(convert.toHtml(text));
} else {
return <>{message}</>;
reactComponent = <>{text}</>;
}

if (logLineSearch) {
reactComponent = HighlightPattern({ text, regex: logLineSearch.pattern, selected });
}
return reactComponent;
};

const HighlightPattern = ({
text,
regex,
selected,
}: {
text: string;
regex: RegExp;
selected: boolean | undefined;
}) => {
Comment on lines +61 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

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 normalizedLogText = normalizeLogText(text);
const splitText = normalizedLogText.split(regex);
const matches = normalizedLogText.match(regex);

if (!isDefined(matches)) {
return <span>{text}</span>;
}

return (
<span>
{splitText.reduce(
(arr: (ReactElement | string)[], element, index) =>
matches[index]
? [
...arr,
element,
<mark key={index}>
{matches[index]}
</mark>,
]
: [...arr, element],
[],
)}
</span>
);
};

return (
<Flex p={"2px 0"} m={"0 16px"} gap={"8px"} alignItems={"top"}>
<Flex p={"2px 0"} m={"0 16px"} gap={"8px"} alignItems={"top"} backgroundColor={selected ? "gray.600" : ""}>
{isDefined(timestamp) && (
<Box
as={"pre"}
Expand All @@ -62,8 +119,9 @@ export const LogLine = ({ timestamp, message, status }: LogLineProps) => {
fontWeight={400}
fontFamily={logFontFamily}
color={statusToColor(status)}
_focus={{ boxShadow: "outline" }}
>
{message && processText(message)}
{message && processText(message, selected)}
</Box>
</Flex>
);
Expand Down
226 changes: 219 additions & 7 deletions enclave-manager/web/src/components/enclaves/logs/LogViewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,30 @@
import { Box, ButtonGroup, Flex, FormControl, FormLabel, Progress, Switch } from "@chakra-ui/react";
import { throttle } from "lodash";
import { ChangeEvent, ReactElement, useEffect, useMemo, useRef, useState } from "react";
import { SmallCloseIcon } from "@chakra-ui/icons";
import {
Box,
Button,
ButtonGroup,
Editable,
EditableInput,
EditablePreview,
Flex,
FormControl,
FormLabel,
HStack,
Input,
InputGroup,
InputRightElement,
Progress,
Switch,
Text,
Tooltip,
} from "@chakra-ui/react";
import { debounce, throttle } from "lodash";
import { ChangeEvent, MutableRefObject, ReactElement, useCallback, useEffect, useMemo, useRef, useState } from "react";
import { Virtuoso, VirtuosoHandle } from "react-virtuoso";
import { isDefined, stripAnsi } from "../../../utils";
import { isDefined, isNotEmpty, stripAnsi } from "../../../utils";
import { CopyButton } from "../../CopyButton";
import { DownloadButton } from "../../DownloadButton";
import { LogLine, LogLineProps } from "./LogLine";
import { LogLine, LogLineProps, LogLineSearch } from "./LogLine";

type LogViewerProps = {
logLines: LogLineProps[];
Expand All @@ -14,6 +33,10 @@ type LogViewerProps = {
logsFileName?: string;
};

export const normalizeLogText = (rawText: string) => {
return rawText.trim();
};

export const LogViewer = ({
progressPercent,
logLines: propsLogLines,
Expand All @@ -24,9 +47,43 @@ export const LogViewer = ({
const [logLines, setLogLines] = useState(propsLogLines);
const [userIsScrolling, setUserIsScrolling] = useState(false);
const [automaticScroll, setAutomaticScroll] = useState(true);

const throttledSetLogLines = useMemo(() => throttle(setLogLines, 500), []);

const searchRef: MutableRefObject<HTMLInputElement | null> = useRef(null);
const [search, setSearch] = useState<LogLineSearch | undefined>(undefined);
const [rawSearchTerm, setRawSearchTerm] = useState("");
const [searchMatchesIndices, setSearchMatchesIndices] = useState<number[]>([]);
const [currentSearchIndex, setCurrentSearchIndex] = useState<number | undefined>(undefined);

useEffect(() => {
window.addEventListener("keydown", function (e) {
const element = searchRef?.current;
if ((e.ctrlKey && e.keyCode === 70) || (e.metaKey && e.keyCode === 70)) {
if (element !== document.activeElement) {
e.preventDefault();
element?.focus();
}
}
// Next search match with cmd/ctrl+G
// if ((e.ctrlKey && e.keyCode === 71) || (e.metaKey && e.keyCode === 71)) {
// console.log("NEXT", e.keyCode);
// e.preventDefault();
// nextMatch();
// }

// Clear the search on escape
if (e.key === "Escape" || e.keyCode === 27) {
if (element === document.activeElement) {
e.preventDefault();
setSearch(undefined);
setRawSearchTerm("");
setSearchMatchesIndices([]);
setCurrentSearchIndex(undefined);
}
}
});
}, []);
Comment on lines +58 to +85
Copy link
Contributor

Choose a reason for hiding this comment

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

This effect should have an unsubscribe return so that the listener is not hanging around when this component is unmounted


useEffect(() => {
throttledSetLogLines(propsLogLines);
}, [propsLogLines, throttledSetLogLines]);
Expand Down Expand Up @@ -54,9 +111,158 @@ export const LogViewer = ({
.join("\n");
};

useEffect(() => {
if (search) findMatches(search);
}, [search?.searchTerm, logLines]);
Comment on lines +114 to +116
Copy link
Contributor

Choose a reason for hiding this comment

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

findMatches is a dependency, but logLines isn't. Also, I try to avoid useEffect that I am expecting to be triggered by a callback (which this is). Instead the callback should just include or call this functionality. If we want this callback to be called on first load then a useEffect with an empty dependency array is fine.

Also I generally try to structure components in roughly this order:

  • use hooks (state and context)
  • useMemo
  • useCallback and functions
  • useEffect
  • jsx (and possibly conditional switches for different content`

I think this makes the code more readable, and less surprising (ie reading top to bottom at this point where did findMatches come from).


const updateSearchTerm = (rawText: string) => {
setCurrentSearchIndex(undefined);
const searchTerm = normalizeLogText(rawText);
const logLineSearch: LogLineSearch = {
searchTerm: searchTerm,
pattern: new RegExp(searchTerm, "gi"), // `i` is invariant case
Copy link
Contributor

Choose a reason for hiding this comment

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

i is case insensitive

};
setSearch(logLineSearch);
};
const debouncedUpdateSearchTerm = debounce(updateSearchTerm, 100);
const debouncedUpdateSearchTermCallback = useCallback(debouncedUpdateSearchTerm, []);
Comment on lines +118 to +128
Copy link
Contributor

Choose a reason for hiding this comment

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

debouncedUpdateSearchTerm is a dependency of debouncedUpdateSearchTermCallback so should be in the dependency array, but it's not. If it were, this would break debouncedUpdateSearchTermCallback as debouncedUpdateSearchTerm is changed on every render, instead it should just be defined in a single useMemo - like:

  const debouncedUpdateSearchTermCallback = useMemo(
    () =>
      debounce((rawText: string) => {
        setCurrentSearchIndex(undefined);
        const searchTerm = normalizeLogText(rawText);
        const logLineSearch: LogLineSearch = {
          searchTerm: searchTerm,
          pattern: new RegExp(searchTerm, "gi"), // `i` is invariant case
        };
        setSearch(logLineSearch);
      }, 100),
    [],
  );


const hasSearchTerm = () => {
if (!search) return false;
return isDefined(search.searchTerm) && isNotEmpty(search.searchTerm);
Comment on lines +131 to +132
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit):

return isDefined(search?.searchTerm) && isNotEmpty(search.searchTerm);

};

const findMatches = (search: LogLineSearch) => {
setSearchMatchesIndices([]);
if (hasSearchTerm()) {
const matches = logLines.flatMap((line, index) => {
if (line?.message && normalizeLogText(line.message).match(search.pattern)) {
return index;
} else {
return [];
}
});
setSearchMatchesIndices(matches);
}
};

const handleOnChange = (e: ChangeEvent<HTMLInputElement>) => {
setRawSearchTerm(e.target.value);
debouncedUpdateSearchTermCallback(e.target.value);
};

const priorMatch = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

(very minor) wasn't immediately obvious to me this is a function - should be named something like handlePriorMatchClick

if (searchMatchesIndices.length > 0) {
const newIndex = isDefined(currentSearchIndex) ? currentSearchIndex - 1 : 0;
updateSearchIndexBounded(newIndex);
}
};

const nextMatch = () => {
if (searchMatchesIndices.length > 0) {
const newIndex = isDefined(currentSearchIndex) ? currentSearchIndex + 1 : 0;
updateSearchIndexBounded(newIndex);
}
};

const updateSearchIndexBounded = (newIndex: number) => {
if (newIndex > searchMatchesIndices.length - 1) {
newIndex = 0;
}
if (newIndex < 0) {
newIndex = searchMatchesIndices.length - 1;
}
setCurrentSearchIndex(newIndex);
return newIndex;
};

useEffect(() => {
if (virtuosoRef?.current && currentSearchIndex !== undefined && currentSearchIndex >= 0) {
virtuosoRef.current.scrollToIndex(searchMatchesIndices[currentSearchIndex]);
}
}, [currentSearchIndex]);
Comment on lines +179 to +183
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be a useEffect and should probably be either in a callback or called direct from the navigation/find matches code.


const clearSearch = () => {
setRawSearchTerm("");
setSearch(undefined);
setSearchMatchesIndices([]);
setCurrentSearchIndex(undefined);
};

const parseMatchIndexRequest = (input: string) => {
let parsed = parseInt(input);
if (isNaN(parsed) || parsed < 1) return 1;
if (parsed > searchMatchesIndices.length) return searchMatchesIndices.length;
return parsed;
};

const highlight = (currentSearchIndex: number | undefined, thisIndex: number, searchableIndices: number[]) => {
return (
currentSearchIndex !== undefined &&
searchableIndices.length > 0 &&
searchableIndices[currentSearchIndex] === thisIndex
);
};

return (
<Flex flexDirection={"column"} gap={"32px"} h={"100%"}>
<Flex flexDirection={"column"} position={"relative"} bg={"gray.800"} h={"100%"}>
<Box width={"100%"}>
<Flex m={4}>
<Flex width={"40%"}>
<InputGroup size="sm">
<Input
size={"sm"}
ref={searchRef}
value={rawSearchTerm}
onChange={handleOnChange}
placeholder={"search"}
/>
{rawSearchTerm && (
<InputRightElement>
<SmallCloseIcon onClick={clearSearch} />
</InputRightElement>
)}
</InputGroup>
</Flex>
<Button size={"sm"} ml={2} onClick={priorMatch}>
Previous
</Button>
<Button size={"sm"} ml={2} onClick={nextMatch}>
Next
</Button>
{hasSearchTerm() && (
<Box ml={2}>
<Text align={"left"} color={searchMatchesIndices.length === 0 ? "red" : "kurtosisGreen.400"}>
<HStack alignItems={"center"}>
<>
{searchMatchesIndices.length > 0 && currentSearchIndex !== undefined && (
<>
<Editable
p={0}
m={0}
size={"sm"}
value={`${currentSearchIndex + 1}`}
onChange={(inputString) =>
updateSearchIndexBounded(parseMatchIndexRequest(inputString) - 1)
}
>
Comment on lines +241 to +249
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with this component, but wondering if we can apply numeric constraint to it.

<Tooltip label="Click to edit" shouldWrapChildren={true}>
<EditablePreview />
</Tooltip>
<EditableInput p={1} width={"50px"} />
</Editable>
<>/ </>
</>
)}
<>{searchMatchesIndices.length} matches</>
</>
</HStack>
</Text>
</Box>
)}
</Flex>
</Box>
{isDefined(ProgressWidget) && (
<Box
display={"inline-flex"}
Expand Down Expand Up @@ -84,7 +290,13 @@ export const LogViewer = ({
isScrolling={setUserIsScrolling}
style={{ height: "100%" }}
data={logLines.filter(({ message }) => isDefined(message))}
itemContent={(_, line) => <LogLine {...line} />}
itemContent={(index, line) => (
<LogLine
logLineProps={line}
logLineSearch={search}
selected={highlight(currentSearchIndex, index, searchMatchesIndices)}
/>
)}
/>
{isDefined(progressPercent) && (
<Progress
Expand Down
4 changes: 4 additions & 0 deletions enclave-manager/web/src/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ export function isDefined<T>(it: T | null | undefined): it is T {
return it !== null && it !== undefined;
}

export function isNotEmpty(it: string): it is string {
return it.length > 0;
}

export function isStringTrue(value?: string | null) {
return (value + "").toLowerCase() === "true";
}
Expand Down
6 changes: 3 additions & 3 deletions engine/server/webapp/asset-manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"files": {
"main.js": "./static/js/main.14d7f9cc.js",
"main.js": "./static/js/main.bab03440.js",
"index.html": "./index.html",
"main.14d7f9cc.js.map": "./static/js/main.14d7f9cc.js.map"
"main.bab03440.js.map": "./static/js/main.bab03440.js.map"
},
"entrypoints": [
"static/js/main.14d7f9cc.js"
"static/js/main.bab03440.js"
]
}
Loading