[Infra UI] Add new graphql endpoint for snapshot data #34264

skh · 2019-04-01T13:43:28Z

Summary

First part of implementation for #31405 -- add a new graphql endpoint with the new behaviour.

This is mostly a rewrite of the existing nodes endpoint. Once this change has been merged, the UI will be changed to use this new endpoint, and the old one can be removed. This will happen in #34938 .

Notable changes:

Simplified source code structure: no separation of adapter and domain code as we agreed we don't need this abstraction any more
Use of composite aggregation instead of terms aggregation with partitions. By this, generating the ES queries could be simplified.
Removed the option to group by filters (instead of fields) as it isn't used by the UI at all
Namespaced types that are only used in this part to InfraSnapshot*
Reduced the number of explicit types as much as possible

Testing

Test in GraphiQL.

Sample query (replace FROM, TO and HOSTNAME)

{
  source(id: "default") {
    snapshot (timerange: {from:FROM,interval:"1m",to:TO}, filterQuery:"{\"bool\":{\"should\":[{\"match_phrase\":{\"host.name\":\"HOSTNAME\"}}],\"minimum_should_match\":1}}") {
      nodes(type: host, groupby: [{field:"service.type", label:"Service"},{field:"agent.version"}], metric: {type: tx} ) {
        path {
          value
          label
        }
        metric {
          value
          avg
          max
          name
        }
      }
    }
  }
}

have test data from at least two nodes, if possible
set SNAPSHOT_COMPOSITE_REQUEST_SIZE in server/lib/snapshot/constants.ts to a number smaller than your number of nodes
enable debug logging (in your Kibana config, set elasticsearch.logQueries: true and logging.verbose: true) to verify that multiple queries are indeed performed
test various groupBys, metric types and filter queries

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~

~~- [ ] Unit or functional tests were updated or added to match the most common scenarios~~

I would like to add tests in the implementation of #34718 because we use the query definitions from the UI containers in our functional tests, and this is where they will be added.

~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

This is ready for review now, or together with #34938 -- whichever you prefer.

elasticmachine · 2019-04-01T14:41:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-01T14:41:55Z

Pinging @elastic/infrastructure-ui

elasticmachine · 2019-04-04T13:24:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-09T14:49:20Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-09T15:28:02Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-09T15:42:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-09T16:34:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-11T13:33:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-15T09:31:59Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

skh · 2019-04-15T09:33:56Z

jenkins, please test this

elasticmachine · 2019-04-15T10:23:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

simianhacker

I left some notes inline with the code...

Also, I would like to see an integration test for this as well. It looks like the original map test is tied directly with the UI query which makes it difficult to copy. I would probably copy it and modify it to use a hard coded query. Then update it to be consistent once the UI portion is available. I would leave a TODO comment next to the hard coded query OR file a follow up issue so we don't forget.

simianhacker · 2019-04-15T19:24:13Z

x-pack/plugins/infra/server/lib/snapshot/snapshot.ts

+};
+
+const mergeNodeMetrics = (
+  nodes: any[],


Both nodes and metrics should use InfraSnapshotMetricResponse[] as the type. Also I would change the names to nodeBuckets and nodeBucketsWithMetrics.

They have different types, but you're right nevertheless.

simianhacker · 2019-04-15T19:26:05Z

x-pack/plugins/infra/server/lib/snapshot/snapshot.ts

+  const result: any[] = [];
+  const nodeMetricsForLookup = getNodeMetricsForLookup(metrics);
+
+  nodes.forEach(node => {


Please change this to a map so that it doesn't side effect result and just return directly from the map. This will eliminate the any on line 186 as well.

simianhacker · 2019-04-15T19:27:14Z

x-pack/plugins/infra/server/lib/snapshot/snapshot.ts

+  return buckets;
+};
+
+const mergeNodeMetrics = (


How about renaming this to mergeNodeBuckets?

simianhacker · 2019-04-15T19:39:27Z

x-pack/plugins/infra/server/lib/snapshot/snapshot.ts

+    },
+  };
+
+  let response = await framework.callWithRequest<any, any>(request, 'search', query);


Lines 94-108 are identical to lines 164-178. How about we create a function named getAllNodeBuckets which return InfraSnapshotMetricResponse[]? I also think we need to defined the Aggregation response type and pass it to the second type argument on the function call, something like:

interface InfraSnapshotAggregation { buckets: InfraSnapshotMetricResponse[]; after_key: { [id: string]: string }; } interface InfraSnapshotAggregationResponse { nodes: InfraSnapshotAggregation; } const response = framework.callWithRequest<{}, InfraSnapshotAggregationResponse>(request, 'search', query);

When you provide an aggregation type, the generics that back up that request will return a type that defines the aggregation key as optional which will then require a check to make sure aggregations are return. We can't count on the response to always have that key set.

I would also like to see the let and side effecting while loop factored out of this code. If you combine lines 94-108 and 164-178 in to one function. You could write that function to recurse until there are no more results, then return all the buckets. Maybe something like this:

const getAllNodesBuckets = async ( framework: InfraBackendFrameworkAdapter, req: InfraFrameworkRequest, requestQuery: InfraSnapshotNodeRequest, // this type needs to be created previousBuckets: InfraSnapshotMetricResponse[] = [] ): Promise<InfraSnapshotMetricResponse[]> => { const response = await framework.callWithRequest<{}, InfraSnapshotAggregationResponse>( req, 'search', requestQuery ); // Nothing available, return the previous buckets. if (response.hits.total.value === 0) { return previousBuckets; } // if ES doesn't return aggregations key, we can either throw an error or return previousBuckets. if (!response.aggregations) { throw new Error('Whoops!, `aggregations` key must always be returned.'); } const currentBuckets = response.aggregations.nodes.buckets; // if the buckets length is zero then we are finished paginating through the results if (currentBuckets.length === 0) { return previousBuckets; } // create a new request object const newQuery = { ...requestQuery, body: { ...requestQuery.body, aggs: { ...requestQuery.body.aggs, nodes: { ...requestQuery.body.aggs.nodes, composite: { ...requestQuery.body.aggs.nodes.composite, after: response.aggregations.nodes.after_key, }, }, }, }, }; // request additional buckets return getAllNodesBuckets(framework, req, newQuery, [...previousBuckets, ...currentBuckets]); };

Makes sense. Thanks.

simianhacker · 2019-04-15T20:06:52Z

x-pack/plugins/infra/server/lib/snapshot/response_helpers.ts

+import { getIntervalInSeconds } from '../../utils/get_interval_in_seconds';
+import { InfraSnapshotRequestOptions } from './snapshot';
+
+export interface InfraSnapshotMetricResponse {


How about we rename this to InfraSnapshotNodeBucket

Good point. I'll refine the type names in that file.

simianhacker · 2019-04-15T20:44:11Z

x-pack/plugins/infra/server/lib/snapshot/query_helpers.ts

+
+export const getGroupedNodesSources = (options: InfraSnapshotRequestOptions) => {
+  const sources = [];
+  options.groupby.forEach(gb => {


How about we use a map here with a trailing push for the last source definition and then return the results directly from the function? This needs a return type as well.

Agreed on the map, but I'm not sure about the return type -- this function is used only once and only factored out for readability. Adding an explicit type only adds mental overload here in my opinion.

I only recommended the type because Typescript will likely complain about the object/key signature. If it works without the type then it should be fine.

simianhacker · 2019-04-15T20:50:58Z

x-pack/plugins/infra/server/lib/snapshot/response_helpers.ts

+) => {
+  const path = [];
+  const node = groupBucket.key;
+  options.groupby.forEach(gb => {


Change this to a map with a push for the trailing value and return directly from the function.

simianhacker · 2019-04-15T20:51:06Z

x-pack/plugins/infra/server/lib/snapshot/response_helpers.ts

+
+export const getNodeMetricsForLookup = (metrics: InfraSnapshotMetricResponse[]) => {
+  const nodeMetricsForLookup: any = {};
+  metrics.forEach(metric => {


How about we use a reduce here to transform the array into a hash? Also we should define a return type for the parent function.

simianhacker · 2019-04-15T21:06:28Z

x-pack/plugins/infra/server/graphql/snapshot/schema.gql.ts

+    "Nodes of type host, container or pod grouped by 0, 1 or 2 terms"
+    nodes(
+      type: InfraNodeType!
+      groupby: [InfraSnapshotGroupbyInput!]!


I would change groupby to groupBy to be consistent

Suggested change

groupby: [InfraSnapshotGroupbyInput!]!

groupBy: [InfraSnapshotGroupbyInput!]!

elasticmachine · 2019-04-17T13:37:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-17T14:45:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

skh · 2019-04-18T10:57:21Z

Also, I would like to see an integration test for this as well. It looks like the original map test is tied directly with the UI query which makes it difficult to copy. I would probably copy it and modify it to use a hard coded query. Then update it to be consistent once the UI portion is available. I would leave a TODO comment next to the hard coded query OR file a follow up issue so we don't forget.

That's why I wrote in the original description:

I would like to add tests in the implementation of #34718 because we use the query definitions from the UI containers in our functional tests, and this is where they will be added.

I'm actually already working on these tests, but it will be easier to add them in the next PR (which is based on this one). Would this work for you?

elasticmachine · 2019-04-18T11:53:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

skh · 2019-04-18T12:10:42Z

@simianhacker thanks for your review! I've responded to all your comments, this is ready for another look.

simianhacker

Thanks for the changes. Good Job!

 ___       ________ _________  _____ ______      
|\  \     |\   ____\\___   ___\\   _ \  _   \    
\ \  \    \ \  \___\|___ \  \_\ \  \\\__\ \  \   
 \ \  \    \ \  \  ___  \ \  \ \ \  \\|__| \  \  
  \ \  \____\ \  \|\  \  \ \  \ \ \  \    \ \  \ 
   \ \_______\ \_______\  \ \__\ \ \__\    \ \__\
    \|_______|\|_______|   \|__|  \|__|     \|__|

* Add new graphql endpoint for snapshot data * Polishing. * Keep type generic that is used outside snapshots * Keep one more generic type generic * Use camelCase for consistency. * Refine type names * Add return types. * Use idiomatic javascript. * Factor out getAllCompositeAggregationData<T>() * Refine naming. * More idiomatic JavaScript, more types.

…35792) * [Infra UI] Add new graphql endpoint for snapshot data (#34264) * Add new graphql endpoint for snapshot data * Polishing. * Keep type generic that is used outside snapshots * Keep one more generic type generic * Use camelCase for consistency. * Refine type names * Add return types. * Use idiomatic javascript. * Factor out getAllCompositeAggregationData<T>() * Refine naming. * More idiomatic JavaScript, more types. * Use pre-8.x response format (see also #30826)

skh self-assigned this Apr 1, 2019

skh force-pushed the infraui-new-snapshot-graphql branch from 9e98ac0 to 04e1ad5 Compare April 1, 2019 14:01

alvarolobato added the Feature:Metrics UI Metrics UI feature label Apr 1, 2019

alvarolobato added [zube]: In Review and removed [zube]: In Review labels Apr 1, 2019

skh force-pushed the infraui-new-snapshot-graphql branch from 04e1ad5 to d4da0f0 Compare April 4, 2019 12:37

skh changed the title ~~Add new graphql endpoint for snapshot data~~ [Infra UI] Add new graphql endpoint for snapshot data Apr 8, 2019

skh mentioned this pull request Apr 8, 2019

[Infra UI] Use new snapshot endpoint for snapshot view #34718

Closed

skh added the v7.2.0 label Apr 8, 2019

Add new graphql endpoint for snapshot data

bca3954

skh force-pushed the infraui-new-snapshot-graphql branch from d4da0f0 to bca3954 Compare April 9, 2019 14:07

Merge branch 'master' into infraui-new-snapshot-graphql

ed6f721

skh force-pushed the infraui-new-snapshot-graphql branch 2 times, most recently from 0043d20 to 469f1c4 Compare April 9, 2019 15:02

Polishing.

065e53b

skh force-pushed the infraui-new-snapshot-graphql branch from 469f1c4 to 065e53b Compare April 9, 2019 15:57

skh added 3 commits April 11, 2019 14:31

Keep type generic that is used outside snapshots

d0d9c95

Merge branch 'master' into infraui-new-snapshot-graphql

8f57fa4

Keep one more generic type generic

5a94704

skh requested a review from simianhacker April 11, 2019 14:42

skh marked this pull request as ready for review April 11, 2019 14:42

zube bot added [zube]: In Progress and removed [zube]: In Review labels Apr 15, 2019

skh added [zube]: In Review and removed [zube]: In Progress labels Apr 15, 2019

simianhacker requested changes Apr 15, 2019

View reviewed changes

skh added 3 commits April 17, 2019 14:34

Use camelCase for consistency.

ad799fb

Refine type names

1d7f049

Add return types.

44838b0

Use idiomatic javascript.

3fa6213

skh added 2 commits April 18, 2019 12:45

Factor out getAllCompositeAggregationData<T>()

78b03a2

Refine naming.

0bc4adc

More idiomatic JavaScript, more types.

c475797

skh force-pushed the infraui-new-snapshot-graphql branch from 9daf16d to c475797 Compare April 18, 2019 11:03

simianhacker approved these changes Apr 18, 2019

View reviewed changes

skh merged commit bff10d2 into elastic:master Apr 18, 2019

zube bot added [zube]: Done and removed [zube]: In Review labels Apr 18, 2019

skh mentioned this pull request Apr 25, 2019

[Infra UI] Use new snapshot endpoint #34938

Merged

2 tasks

alvarolobato removed the [zube]: Done label Apr 29, 2019

skh mentioned this pull request Apr 30, 2019

[7.x] [Infra UI] Add new graphql endpoint for snapshot data (#34264) #35792

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Infra UI] Add new graphql endpoint for snapshot data #34264

[Infra UI] Add new graphql endpoint for snapshot data #34264

skh commented Apr 1, 2019 •

edited

Loading

elasticmachine commented Apr 1, 2019

elasticmachine commented Apr 1, 2019

elasticmachine commented Apr 4, 2019

elasticmachine commented Apr 9, 2019

elasticmachine commented Apr 9, 2019

elasticmachine commented Apr 9, 2019

elasticmachine commented Apr 9, 2019

elasticmachine commented Apr 11, 2019

elasticmachine commented Apr 15, 2019

skh commented Apr 15, 2019

elasticmachine commented Apr 15, 2019

simianhacker left a comment

simianhacker Apr 15, 2019

skh Apr 17, 2019

simianhacker Apr 15, 2019

simianhacker Apr 15, 2019

simianhacker Apr 15, 2019 •

edited

Loading

skh Apr 17, 2019

simianhacker Apr 15, 2019

skh Apr 17, 2019

simianhacker Apr 15, 2019

skh Apr 17, 2019

simianhacker Apr 17, 2019

simianhacker Apr 15, 2019

simianhacker Apr 15, 2019 •

edited

Loading

simianhacker Apr 15, 2019

elasticmachine commented Apr 17, 2019

elasticmachine commented Apr 17, 2019

skh commented Apr 18, 2019 •

edited

Loading

elasticmachine commented Apr 18, 2019

skh commented Apr 18, 2019

simianhacker left a comment

	groupby: [InfraSnapshotGroupbyInput!]!
	groupBy: [InfraSnapshotGroupbyInput!]!

[Infra UI] Add new graphql endpoint for snapshot data #34264

[Infra UI] Add new graphql endpoint for snapshot data #34264

Conversation

skh commented Apr 1, 2019 • edited Loading

Summary

Testing

Checklist

For maintainers

elasticmachine commented Apr 1, 2019

💚 Build Succeeded

elasticmachine commented Apr 1, 2019

elasticmachine commented Apr 4, 2019

💚 Build Succeeded

elasticmachine commented Apr 9, 2019

💔 Build Failed

elasticmachine commented Apr 9, 2019

💔 Build Failed

elasticmachine commented Apr 9, 2019

💔 Build Failed

elasticmachine commented Apr 9, 2019

💚 Build Succeeded

elasticmachine commented Apr 11, 2019

💔 Build Failed

elasticmachine commented Apr 15, 2019

💔 Build Failed

skh commented Apr 15, 2019

elasticmachine commented Apr 15, 2019

💚 Build Succeeded

simianhacker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simianhacker Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simianhacker Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 17, 2019

💚 Build Succeeded

elasticmachine commented Apr 17, 2019

💚 Build Succeeded

skh commented Apr 18, 2019 • edited Loading

elasticmachine commented Apr 18, 2019

💚 Build Succeeded

skh commented Apr 18, 2019

simianhacker left a comment

Choose a reason for hiding this comment

skh commented Apr 1, 2019 •

edited

Loading

simianhacker Apr 15, 2019 •

edited

Loading

simianhacker Apr 15, 2019 •

edited

Loading

skh commented Apr 18, 2019 •

edited

Loading