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

[Infra UI] Consume log entry graphql api in the client #21706

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Aug 6, 2018

New Redux GraphQL helpers

The utils/remote_state/remote_graphql_state.ts file in the client-side code exports a few functions to help with creating redux entities backed by GrahphQL queries:

  • createGraphqlInitialState<State>(initialState: State) to create the basic state structure derived from a generated GraphQL query result
  • createGraphqlOperationActionCreators() to create actions creators relating to those queries
  • createGraphqlOperationReducer() to create a reducer for the action creators
  • createGraphqlQueryEpic() to create an redux-observable epic that manages the side-effects required to resolve the query
  • createGraphqlStateSelectors() to create a set of selectors to inspect the state and results of the query

The entries slice is the first example of usage. (see below)

Redux store changes

entries slice

The entries slice of the redux state now has an entries key that is managed by the GraphQL helpers described above. It defines two queries:

  • load to load and replace the set of entries returned via the Query.source.logEntriesAround fields
  • load_more to load and append more entries to a set of entries already present in the state

Known Limitations and follow-up tasks

This PR has several known limitations:

@weltenwort weltenwort added WIP Work in progress :Ingest UI Feature:Metrics UI Metrics UI feature labels Aug 6, 2018
@weltenwort weltenwort self-assigned this Aug 6, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@weltenwort weltenwort force-pushed the infra-ui-enhancement-graphql-logentry-container branch from b2c371d to 0192aae Compare August 9, 2018 15:43
@weltenwort weltenwort changed the title [Infra UI] [WIP] Consume log entry graphql api in the client [Infra UI] Consume log entry graphql api in the client Aug 9, 2018
@weltenwort weltenwort added review and removed WIP Work in progress labels Aug 9, 2018
@weltenwort weltenwort requested a review from skh August 9, 2018 16:00
@elasticmachine
Copy link
Contributor

💔 Build Failed

@weltenwort weltenwort added the loe:large Large Level of Effort label Aug 9, 2018
@weltenwort weltenwort force-pushed the infra-ui-enhancement-graphql-logentry-container branch from 0192aae to 875ba2e Compare August 9, 2018 18:19
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@weltenwort weltenwort force-pushed the infra-ui-enhancement-graphql-logentry-container branch from f6b131c to ad4f34f Compare August 15, 2018 20:49
@weltenwort
Copy link
Member Author

jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@weltenwort
Copy link
Member Author

unrelated region map test failures again

const index = collectionBisector.left(collection, key);

if (index >= collection.length) {
return;
return null;
}

if (compator(collection[index], key) !== 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

compator looks like a typo?

@@ -6,12 +6,18 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Could logging_legacy be moved to logging at this point?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to skimp on that here because we are about to move these file anyway when moving the redux store to the top level.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, makes sense.


export const createGraphqlInitialState = <State>(initialData?: State): GraphqlState<State> => ({
current: {
progress: 'idle',
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is loading state, could it be named something with loading? progress makes me think of a 0-100% range or something similar and I believe we don't have that.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I will try to streamline this a bit

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, can I do that in a later PR? there are some containers that rely on that structure until #21859. Afterwards refactoring would be much easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure!

selectLoadingResult,
selectLoadingState,
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this way of generating all the redux thingies we will need for a GraphQL endpoint. 👍 No detailed comments right now, I might come back to that when I use it for the first time myself.

@skh
Copy link
Contributor

skh commented Aug 16, 2018

Given the list of known problems in the opening comment I didn't test thoroughly in the UI.

The entries slice of the redux store looks good in Redux Devtools and works as described. In the long run, when the whole Infra UI shares the same store, a more descriptive name for that slice (logEntries maybe?) would be nice but that doesn't need to be done in this PR.

Some minor comments above.

@weltenwort
Copy link
Member Author

I fixed the typo. The slice should definitely be renamed, but I would rather do that as part of the redux store move as well.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@skh
Copy link
Contributor

skh commented Aug 16, 2018

@weltenwort thanks for the fix and the comments.

LGTM 👍

@weltenwort
Copy link
Member Author

region map test failure again, going to merge anyway

@weltenwort weltenwort merged commit 0df8fa8 into elastic:feature-infra-ui Aug 16, 2018
weltenwort added a commit that referenced this pull request Aug 21, 2018
This removes unused code left over after the migration of the log entry api to GraphQL in #21306 and #21706.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Metrics UI Metrics UI feature loe:large Large Level of Effort review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants