-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[ui] Allow filtering run logs for metadata entry keys and values #23438
Conversation
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 496e06d. |
// 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. | ||
// |
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.
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?
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.
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.
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.
Hmmmm I like the weakmap idea, I hadn't thought of the evaluated-per-keystroke implications. Will update this!
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.
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...
Summary & Motivation
Related: https://linear.app/dagster-labs/issue/FE-484/run-logs-search-doesnt-search-through-dictionary-logs
I implemented this in a pretty generic way so that we won't have to revisit this each time we add new events, BUT I think it'd also be fairly straightforward to write a big-ol switch statement that stringifies each unique type, down to switch to that if my "stringify all the metadata entry values" thing is too whacky :-)
How I Tested These Changes
I tested this using a bunch of real-world run logs:
Examples:
A value or the key or both
A value inside "Show JSON"