Skip to content

Conversation

altimin
Copy link
Collaborator

@altimin altimin commented Sep 21, 2025

Leverage the new print_args() function to avoid reconstructing the args tree in JS.

This change doesn't affect UX, just simplifies the code.

@altimin altimin requested a review from a team as a code owner September 21, 2025 17:52
@altimin altimin force-pushed the dev/altimin/print_args branch from 96c7193 to 0b5e8dc Compare September 21, 2025 18:01
@altimin altimin force-pushed the dev/altimin/ui_args branch from c57f6be to 36deba0 Compare September 21, 2025 18:01
@altimin altimin force-pushed the dev/altimin/print_args branch 2 times, most recently from 1f344a9 to 6d07d6e Compare September 21, 2025 19:54
Copy link
Member

@stevegolton stevegolton left a comment

Choose a reason for hiding this comment

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

I think this looks very reasonable in general in terms of the UI side.

That said (and I realize that this PR in particular doesn't change much in this regard, apart from in the ftrace event details panel) but I'm not a huge fan of the fact that the query is abstracted behind the getArgs function.

I'd like to have the option to fetch everything in one go like:

SELECT
   *,
   print_args(arg_set_id) AS argsJson
FROM foo

Then have a synchronous helper to turn this into this into a tree of ArgDict for printing and processing.

In general the SQL helper functions like this usually end up pulling duplicate data and slowing things down. I'll concede that this args example is quite clean but I think as a general principle they should be avoided.

Again as this PR doesn't really change this much (apart from in the ftrace details panel), I think we can tackle this in a different PR, but I thought I'd mention it here.

@altimin altimin force-pushed the dev/altimin/print_args branch from 6d07d6e to 0ed3b10 Compare September 29, 2025 22:40
@altimin altimin force-pushed the dev/altimin/ui_args branch from 36deba0 to b0eb17b Compare September 29, 2025 22:41
@altimin altimin force-pushed the dev/altimin/print_args branch 2 times, most recently from 72f355e to 8ae88f2 Compare September 29, 2025 22:46
@altimin altimin force-pushed the dev/altimin/ui_args branch from b0eb17b to 6e8812a Compare September 29, 2025 22:48
@altimin altimin force-pushed the dev/altimin/print_args branch from 8ae88f2 to 70b4048 Compare October 1, 2025 17:09
Base automatically changed from dev/altimin/print_args to main October 1, 2025 19:38
Leverage the new print_args() function to avoid reconstructing the args tree in JS.
@altimin altimin force-pushed the dev/altimin/ui_args branch from 6e8812a to 7545fb5 Compare October 1, 2025 19:40
@altimin
Copy link
Collaborator Author

altimin commented Oct 2, 2025

Checked with Lalit offline — he is happy with Steve's review here, so I'll land this.

@altimin altimin merged commit bbead82 into main Oct 2, 2025
15 checks passed
@altimin altimin deleted the dev/altimin/ui_args branch October 2, 2025 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants