Don't display and render large execution results, fix lint gulp task and lint violations, use latest up to date docker compose setup for e2e tests #868
Conversation
result but instead display links to view / download raw result. This way we avoid freezing and blocking the browser when trying to render large execution results.
|
Alright, I pushed some more changes, cleanup, etc. The code is probably still non-ideomatic and hacky, but it seems to work. I also tested it with CICD instance for backward compatibility reasons and it also works fine. I did some more testing for the default value for the result size we still render and decided to go with 100 KB. Actual size very much depends on the result type (deeply nested JSON object vs more of a flat one), but 100 KB seems to be a reasonable compromise. Anything beyond that and browser will start to get laggy and slow if result is more of a deeply nested JSON object. In theory, we could have more complex heuristics (e.g. we could display objects up to 1 MB for results which are simple strings), but that would be kinda hard to implement since we would likely need to do it on the client side and likely the logic itself to determine if it's a "deeply nested object" / similar would be somewhat involved. And it's worth noting that I was testing this on a relatively high end PC. On more lower powered devices (also mobile phones), it's likely that even 100 KB is already pushing it for larger JSON objects results. |
end tests with older version of st2.
Kami
changed the title
This allows us to simplify the code and make it even more efficient by only always performing single "get one" execution API operation.
|
Alright, I updated the code to utilize new API query parameter filter from - StackStorm/st2#5197. This allows me to simplify the code and make it even more efficient by always only performing a single "get one execution" API operations (aka win-win). |
|
I see end to end tests use This means end to end tests will fail until 3.5.0 is out since changes in st2web rely on new API added in master. I guess one option would be to update end to end tests to respect version and branching, but this may be a bit of a rabbit hole... EDIT: Not just that, it also uses deprecated st2-docker branch (/~https://github.com/StackStorm/st2-docker/tree/DEPRECATED/all-in-one). So not a rabbit hole I want to touch at this point. |
…orm/st2web into dont_load_large_execution_result
stackstorm/stackstorm image.
|
OK, yeah, I tried to update it to use |
Use st2-docker master docker compose setup for end to end tests
It defaults to false and it can be enabled via config option.
Before that it didn't exit with non-zero so the build didn't fail and tons of violations have accumulated in the code base.
|
I noticed It turns out out gulp task was not configured correctly so it didn't exit with non zero on failure. I fixed that and also fixed existing build violations so the build passes - 65a85b0, f53595b. Going forward we should be much more careful with issues like that. It's much easier to fix violations as they arise vs fixing them later then tons have accumulated... |
Kami
changed the title
|
I'll go ahead and merge this since it's blocking / preventing me from testing things end to end. If there are any comments in the future, I will address them then. And as mentioned above, this PR also contains a bunch of other fixes (lint task, using latest docker compose setup for end to end tests, etc.). |
Add changelog entry for StackStorm/st2web#868.
|
Another page / app which will need similar love in the future (right now it tries to load whole trigger instance payload which may contain large execution result).
But that's very low priority at this point. Unlike history / execution result page, this view is not used all that often. |
Preface / Disclaimer
I'm not a frontend person. I haven't done any serious frontend / JavaScript work for a loooong time. This means that this change is likely sub-optimal, but my main goal is to get it to work without any regressions and negative impact.
Contributions and suggestions from people with more frontend experience are of course very welcome.
Description
The goal of this PR is to solve a long standing issue where st2web would freeze the whole browser window / tab when trying to view execution with large result.
It updates history details component so we don't try to render and display execution result if it's larger than some pre-defined value which can be overridden inside the config (I still need to do some more testing to come up with a reasonable default value for it).
If execution with a larger result is found, we instead display a link where user can either view or download raw result for that execution.
Implementation Details
This pull request depends on the server-side changes from StackStorm/st2#4846.
In that PR (among many other changes), I also added a new
result_sizefield to the each execution DB model. That field holds the size of serialized execution result in bytes.Inside this PR I updated the history details component so we don't fetch execution result by default when fetching execution details. Then inside the action reporter component we check if
result_sizeattribute is present and if it's not larger than the defined threshold (this attribute will only be present for new executions which utilize new DB format - that's intentional since solving it for old executions would mean more code to maintain and it's not worth the effort). If it's not, we re-fetch the whole execution including the result field and display it.Before the result field is loaded, "Loading execution result..." message is displayed to signal the user that the result is being retrieved (honestly with those StackStorm DB changes, users shouldn't see that message all that often or for a long time since those API operations should be much, much faster now).
Ideas for a better implementation
Current implementation means we need to fetch execution twice when viewing the details when result size is smaller than the defined threshold.
This is not ideal, but it's also not the end of the world since with changes from StackStorm/st2#4846 all those API / DB operations will now be much faster.
Technically, we could avoid that additional "get one" query and already fetch result for executions which don't exceed the threshold, but my React foo is not strong enough for that one.
It basically means that history details component would need to have access to the list of all the executions loaded in the left main panel (history panel) and then when retrieving single execution details, we would check if
result_sizefor that execution id in executions list is smaller than the threshold and if it is, retrieve the complete execution including the result.