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

[ui] Allow filtering run logs for metadata entry keys and values #23438

Merged
merged 2 commits into from
Aug 7, 2024
Merged

[ui] Allow filtering run logs for metadata entry keys and values #23438

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec6b520
[ui] Allow filtering run logs for metadata entry keys and values
Aug 6, 2024
496e06d
PR feedback
Aug 6, 2024
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
23 changes: 15 additions & 8 deletions js_modules/dagster-ui/packages/ui-core/src/assets/AssetEventMetadataEntriesTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {Timestamp} from '../app/time/Timestamp';
import {
HIDDEN_METADATA_ENTRY_LABELS,
MetadataEntry,
MetadataEntryLabelOnly,
isCanonicalRowCountMetadataEntry,
} from '../metadata/MetadataEntry';
import {
Expand Down Expand Up @@ -135,14 +136,7 @@ export const AssetEventMetadataEntriesTable = ({
() =>
allRows
.filter((row) => !filter || row.entry.label.toLowerCase().includes(filter.toLowerCase()))
.filter(
(row) =>
!HIDDEN_METADATA_ENTRY_LABELS.has(row.entry.label) &&
!(isCanonicalColumnSchemaEntry(row.entry) && hideTableSchema) &&
!isCanonicalColumnLineageEntry(row.entry) &&
!isCanonicalRowCountMetadataEntry(row.entry) &&
!isCanonicalCodeSourceEntry(row.entry),
),
.filter((row) => !isEntryHidden(row.entry, {hideTableSchema})),
[allRows, filter, hideTableSchema],
);

Expand Down Expand Up @@ -331,3 +325,16 @@ export const StyledTableWithHeader = styled.table`
}
}
`;

function isEntryHidden(
entry: MetadataEntryLabelOnly,
{hideTableSchema}: {hideTableSchema: boolean | undefined},
) {
return (
HIDDEN_METADATA_ENTRY_LABELS.has(entry.label) ||
(isCanonicalColumnSchemaEntry(entry) && hideTableSchema) ||
isCanonicalColumnLineageEntry(entry) ||
isCanonicalRowCountMetadataEntry(entry) ||
isCanonicalCodeSourceEntry(entry)
);
}
24 changes: 23 additions & 1 deletion js_modules/dagster-ui/packages/ui-core/src/runs/filterLogs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {LogFilter, LogsProviderLogs} from './LogsProvider';
import {eventTypeToDisplayType} from './getRunFilterProviders';
import {logNodeLevel} from './logNodeLevel';
import {LogNode} from './types';

export function filterLogs(logs: LogsProviderLogs, filter: LogFilter, filterStepKeys: string[]) {
const filteredNodes = logs.allNodes.filter((node) => {
Expand All @@ -26,6 +27,7 @@ export function filterLogs(logs: LogsProviderLogs, filter: LogFilter, filterStep

const textMatchNodes = hasTextFilter
? filteredNodes.filter((node) => {
const nodeTexts = [node.message.toLowerCase(), ...metadataEntryKeyValueStrings(node)];
return (
filter.logQuery.length > 0 &&
filter.logQuery.every((f) => {
Expand All @@ -38,7 +40,7 @@ export function filterLogs(logs: LogsProviderLogs, filter: LogFilter, filterStep
if (f.token === 'type') {
return node.eventType && f.value === eventTypeToDisplayType(node.eventType);
}
return node.message.toLowerCase().includes(f.value.toLowerCase());
return nodeTexts.some((text) => text.toLowerCase().includes(f.value.toLowerCase()));
bengotow marked this conversation as resolved.
Show resolved Hide resolved
})
);
})
Expand All @@ -49,3 +51,23 @@ export function filterLogs(logs: LogsProviderLogs, filter: LogFilter, filterStep
textMatchNodes,
};
}

// Given an array of metadata entries, returns searchable text in the format:
// [`label1:value1`, ...], where "value1" is the top-level value(s) present in the
// metadata entry object converted to JSON. All of our metadata entry types contain
// different top-level keys, such as "intValue", "mdStr" and "tableSchema", and
// the searchable text is the value of these keys.
//
Comment on lines +57 to +62
Copy link
Member

Choose a reason for hiding this comment

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

How big can the metadataEntries object be? Could this stringifying and searching become sluggish (or memory intensive, if we're constantly stringifying everything, e.g. on every keystroke) for large metadata sets on runs with zillions of logs?

Copy link
Contributor

@salazarm salazarm Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Similar perf concern from me. I think we only need to stringify once for each message. We could accomplish that by using a weakmap to memoize metadataEntryKeyValueStrings, or creating a new version of the object with the extra string field on it. I would prefer the former.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmmm I like the weakmap idea, I hadn't thought of the evaluated-per-keystroke implications. Will update this!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wrapped this in our weakmapMemoize helper and used some console.logs in the body of the function to verify that it runs on the first keystroke and not on subsequent keystrokes 🙌 Good call! I think that would have been bad...

function metadataEntryKeyValueStrings(node: LogNode) {
if (!('metadataEntries' in node)) {
return [];
}
const s = node.metadataEntries.map(
// eslint-disable-next-line unused-imports/no-unused-vars
({__typename, label, description, ...rest}) =>
bengotow marked this conversation as resolved.
Show resolved Hide resolved
`${label}:${Object.values(rest)
.map((v) => (typeof v === 'string' ? v : JSON.stringify(v)))
.join(' ')}`,
);
return s;
}