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.

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

[ML] Adds redux toolkit example for response_stream to developer examples. #182690

Merged
merged 25 commits into from
May 22, 2024

Conversation

walterra
Copy link
Contributor

@walterra walterra commented May 6, 2024

Summary

Follow up to #132590.
Part of #181111.

This updates the developer examples for @kbn/ml-response-stream to include a variant with a full Redux Toolkit setup. For this case, the @kbn/ml-response-stream now includes a generic slice streamSlice that can be used. This allows the actions created to be streamed via NDJSON to be shared across server and client.

Functional tests for the examples were added too. To run these tests you can use the following commands:

# Start the test server (can continue running)
node scripts/functional_tests_server.js --config test/examples/config.js
# Start a test run
node scripts/functional_test_runner.js --config test/examples/config.js

Checklist

@walterra walterra self-assigned this May 6, 2024
@walterra walterra force-pushed the ml-response-stream-createslice branch from 10f9f2f to ed30783 Compare May 6, 2024 17:43
@walterra walterra force-pushed the ml-response-stream-createslice branch from 8105d5d to 9946011 Compare May 15, 2024 08:18
@walterra walterra added release_note:skip Skip the PR/issue when compiling release notes v8.15.0 :ml labels May 15, 2024
@elastic elastic deleted a comment from kibana-ci May 15, 2024
@walterra walterra force-pushed the ml-response-stream-createslice branch from 138324e to 7dfe90b Compare May 16, 2024 07:52
@elastic elastic deleted a comment from kibana-ci May 16, 2024
@walterra walterra marked this pull request as ready for review May 16, 2024 11:40
@walterra walterra requested a review from a team as a code owner May 16, 2024 11:40
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@walterra walterra force-pushed the ml-response-stream-createslice branch from 7992dbc to c03a427 Compare May 16, 2024 11:44
const randomEntity = entities[Math.floor(Math.random() * entities.length)];
const randomAction = actions[Math.floor(Math.random() * actions.length)];

if (randomAction === 'add') {
Copy link
Member

Choose a reason for hiding this comment

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

super duper nit: I think a switch statement would be more readable here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 737bdfd.

@@ -58,7 +49,7 @@ export const PageReducerStream: FC = () => {
RESPONSE_STREAM_API_ENDPOINT.REDUCER_STREAM,
'1',
{ compressResponse, flushFix, simulateErrors },
{ reducer: reducerStreamReducer, initialState }
{ reducer: reducerStreamReducer, initialState: getInitialState() }
Copy link
Member

Choose a reason for hiding this comment

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

getInitialState() will be called every render here even though it's already been used to create the reducer initially. Is that what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 73d148a.

Copy link
Member

@qn895 qn895 left a comment

Choose a reason for hiding this comment

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

Code LGTM 🎉

deleteEntity: (state, action: PayloadAction<string>) => {
delete state.entities[action.payload];
},
error: (state, action: PayloadAction<string>) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a way to clear out errors other than the reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The example UI doesn't have an option for that, so it's not necessary.

@alvarezmelissa87
Copy link
Contributor

Looks good overall. The only thing I noticed when using the example stream was that, when the 'simulate error' box was checked the error toasts only showed up sometimes. The stream always stopped at some point but the error toast only showed up maybe every other time. Saw this for both the Redux Toolkit stream and the useReducer stream. Not sure if that's expected.

image

@walterra
Copy link
Contributor Author

walterra commented May 22, 2024

@alvarezmelissa87 great question and finding about an error not triggering a toast. Had to read/understand again myself since it's been a while since I wrote that pattern.

On the server side we have these two parts:

              case 'throw-error':
                // Throw an error. It should not crash Kibana!
                // It should be caught and logged to the Kibana server console.
                throw new Error('There was a (simulated) server side error!');

              case 'emit-error':
                // Emit an error as a stream action.
                push(error('(Simulated) error pushed to the stream'));
                return;

So the first case throws an error server side, but it will not be propagated to the client side, because a HTTP stream can only return an error on the initial return (that would be something instead of return response.ok(responseWithHeaders);, for example if a user doesn't have enough permissions or similar). You'll just see an error on the Kibana server log via throw. The code for this example plugin is written this way to highlight the different behaviors for an error that's thrown and one that's pushed to the stream. I updated the comments in the example code to clarify.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
aiops 610 612 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @walterra

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

LGTM ⚡

@walterra walterra merged commit 5345e34 into elastic:main May 22, 2024
18 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 22, 2024
@walterra walterra deleted the ml-response-stream-createslice branch May 22, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting :ml release_note:skip Skip the PR/issue when compiling release notes v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants