-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Ingest UI] WaffleMap #21871
[Ingest UI] WaffleMap #21871
Conversation
💚 Build Succeeded |
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.
Thanks for moving over the files. I've commented on a few problems with the usage of the generated query types and the schema below.
InfraSource, | ||
InfraTimerangeInput, | ||
} from '../../../common/graphql/types'; | ||
import { InfraWaffleMapGroup } from '../../lib/lib'; | ||
import { nodesToWaffleMap } from './nodes_to_wafflemap'; | ||
import { query } from './query'; |
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.
The query imported here is named Query
. That's a very conflict-prone name, especially since it will end up in the generated types. How about we call it something like
export const mapQuery = gql`
query MapQuery(...) {
...
}
`;
Then the generated query namespace would be MapQuery
in common/graphql/types.ts
(with the query result type as MapQuery.Query
and the variables as MapQuery.Variables
, see below).
props: ({ data }) => ({ map: (data && data.source && data.source.map) || {} }), | ||
props: ({ data }) => { | ||
const nodes = get(data, 'source.map.nodes', []); | ||
return { map: nodesToWaffleMap(nodes) }; |
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.
According to the schema source.map.nodes
can be null
, which means the nodesToWaffleMap
function fails at runtime when the query does not yield any nodes:
The compiler would have warned about this if instead of the hand-written Response
, the generated Query.Query
(or MapQuery.Query
after the proposed renaming above) from common/graphql/types.ts
had been used.
How about fixing that by
- use the generated query response type and
- change the schema and resolver of
InfraResponse
to always return an array fornodes
(empty when no nodes are found)?
The same goes for the Variables
. Let's use the generated Query.Variables
(or MapQuery.Variables
).
@weltenwort I implemented the changes you requested. The |
💔 Build Failed |
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.
When we supply an id
, apollo-graphql uses it to identify the entity in its cache, so later responses containing that id will overwrite any previous one with the same key. Not sure how it will merge new data coming in when we implement the polling. It's one of the reasons why I want to put the storage logic into the redux store - it's easier to control the merging. I think for our purposes it might be most beneficial to not have ids in the hierarchy beneath maps()
, but to associate a fixed key with the whole query using the @connection
directive. 🤔
@@ -38,10 +33,10 @@ interface Variables { | |||
|
|||
export const withMap = graphql< | |||
{}, // OptionProps, this will end up being the options that contain index pattern, filters, etc | |||
Response, | |||
MapQuery.Query, | |||
Variables, |
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.
There is also a generated variable type in MapQuery.Variables
.
const { nodes } = data.source.map; | ||
if (!nodes) { | ||
return emptyResponse; | ||
} |
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.
I usually express that as
props: ({ data }) => ({
map:
data && data.source && data.source.map && data.source.map.nodes
? nodesToWaffleMap(data.source.map.nodes)
: [],
}),
I find that more readable, but that's obviously a matter of taste and habit. What do you think?
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.
When we supply an id
, apollo-graphql uses it to identify the entity in its cache, so later responses containing that id will overwrite any previous one with the same key. Not sure how it will merge new data coming in when we implement the polling. I would expect to land in a weird mix of states between old and new data. It's one of the reasons why I want to put the storage logic into the redux store - it's easier to control the merging.
I think for our purposes it might be most beneficial to not have ids in the hierarchy beneath maps()
, but to associate a fixed key with the whole map()
query using the @connection
directive. 🤔
edit: no idea why this was submitted twice, sorry
💔 Build Failed |
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.
Seems to work with a no-cache
fetchPolicy
using the newer apollo-client version 👍 And with the removal of the id
s the path
argument is a lot lighter too.
I think this is mostly ready to be merged to bring us forward in the feature branch. I just left a small comment about the bucket name handling below.
const secondGroup: InfraPathInput = groupBy[1]; | ||
const paths: InfraNode[] = node[firstGroup!.id].buckets.reduce( | ||
const paths: InfraNode[] = node.path_0.buckets.reduce( | ||
(acc: InfraNode[], bucket: InfraBucket, index: number): InfraNode[] => { | ||
const nodeItem = createNodeItem(options, node, bucket); | ||
const key: string = String(bucket.key || index); | ||
if (secondGroup) { |
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.
Should this be node.path_1
for consistency?
jenkins did not trigger a test run 🤔 jenkins, test this, please |
💚 Build Succeeded |
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.
Could we use a less generic name than Container
for the outer styled component, and could we use a consistent schema?
Right now, some of them have a name indicating what they are styling, e.g. GroupOfNodesContainer
, but a generic StyledContainer
(or whatever) that's the same in each component would probably work as well.
); | ||
}; | ||
|
||
export const Container = styled.div` |
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.
Could we use a less generic name for 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.
What name would have made more sense here?
Keep in mind I think that name is only visible in this file. It doesn't leak outside the file anywhere else, those styled components are isolated to the file. If you look at the page with the React dev tool it shows up under a different name. It was a mistake to export
those containers, force of habit :D
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.
We could follow the naming convention you used in the other components, so it would be a GroupOfGroupsContainer
in this case. (See also the review comment above.)
}; | ||
} | ||
|
||
const Container = styled.div` |
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.
Could we use a less generic name for 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.
See above
}; | ||
} | ||
|
||
export const Container = styled.div` |
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.
Could we use a less generic name for 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.
See above
import { InfraWaffleData, InfraWaffleMapGroup, InfraWaffleOptions } from '../../lib/lib'; | ||
import { applyWaffleMapLayout } from './lib/apply_wafflemap_layout'; | ||
|
||
interface WafleMapProps { |
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.
Typo: WaffleMapProps
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.
Good catch
…r/kibana into feature-infra-ui-wafflemap-2
@skh I updated the names to be more specific. I also removed all the exports since I never intended those to be used outside the context of that component. @weltenwort We should be ready |
jenkins, test 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.
Jenkins seems unwilling to run the tests at the moment. That aside I'd say merging this would bring us forward a lot 👍 Great job!
ah, it's running now 🎉 |
💔 Build Failed |
looks like unrelated test failures related to the region map |
Merging... since we are just merging into our own feature branch, if it something we broke we can fix it later. Moving on. |
🎉 |
After pulling in upstream I realized that this PR was also damaged in yesterday's VS Code/Git/Merge snafu. The original PR was located at #21744
This PR adds the basic functionality for the Waffle Map to the Infra UI. Currently there is nothing configurable via the UI. The options are hard coded in the
withMap
container query, this will change as the UI features are developed to support it.How to test in browser:
demo-stack
Kibana instance viaconfig/kibana.dev.yml
(reach out to @simianhacker for actual configuration)