[Monitoring] Only do a single date_histogram agg for get_nodes calls

[chrisronline]

Resolves #43477

This PR changes the query executed when we fetch the nodes listing page from within the Stack Monitoring UI.

Currently, the query does multiple sub date_histogram aggregations, when only one is necessary. Unfortunately, this change is more complicated as parts of the code flow involved touch multiple shared areas of code, not easily changed to support this PR. As a result, we have a bit of shuffling and unshuffling happening to ensure we can perform a single date_histogram query, yet also return the same, expected structure to the rest of the monitoring code.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

Pinging @elastic/stack-monitoring

[cachedout]

Looked like :)

[cachedout]

This looks reasonably straightforward and the gains are pretty obvious. Are you thinking that down the road we'd continue by changing the rest of the code to avoid having to do this convert/unconvert strategy?

[chrisronline]

This looks reasonably straightforward and the gains are pretty obvious. Are you thinking that down the road we'd continue by changing the rest of the code to avoid having to do this convert/unconvert strategy?

I don't think it's that simple.

Historically, the aggregation part of the query looks like (I intentionally removed a few of the aggregations to shorten it):

{
  "aggs": {
    "nodes": {
      "terms": {
        "field": "source_node.uuid",
        "size": 10000
      },
      "aggs": {
        "node_cgroup_quota": {
          "date_histogram": {
            "field": "timestamp",
            "min_doc_count": 1,
            "fixed_interval": "30s"
          },
          "aggs": {
            "usage": {
              "max": {
                "field": "node_stats.os.cgroup.cpuacct.usage_nanos"
              }
            },
            "periods": {
              "max": {
                "field": "node_stats.os.cgroup.cpu.stat.number_of_elapsed_periods"
              }
            },
            "quota": {
              "min": {
                "field": "node_stats.os.cgroup.cpu.cfs_quota_micros"
              }
            },
            "usage_deriv": {
              "derivative": {
                "buckets_path": "usage",
                "gap_policy": "skip",
                "unit": "1s"
              }
            },
            "periods_deriv": {
              "derivative": {
                "buckets_path": "periods",
                "gap_policy": "skip",
                "unit": "1s"
              }
            }
          }
        },
        "node_cgroup_throttled": {
          "date_histogram": {
            "field": "timestamp",
            "min_doc_count": 1,
            "fixed_interval": "30s"
          },
          "aggs": {
            "metric": {
              "max": {
                "field": "node_stats.os.cgroup.cpu.stat.time_throttled_nanos"
              }
            },
            "metric_deriv": {
              "derivative": {
                "buckets_path": "metric",
                "unit": "1s"
              }
            }
          }
        }
      }
    }
  }
}

The code then takes out the named aggregation buckets, which makes each aggregation structure look like (again, removing redundant parts):

"node_cgroup_throttled": {
  "buckets": [
    {
      "key_as_string": "2019-08-21T14:27:00.000Z",
      "key": 1566397620000,
      "doc_count": 3,
      "metric": {
        "value": null
      },
      "metric_deriv": {
        "value": null,
        "normalized_value": null
      }
    },
    {
      "key_as_string": "2019-08-21T14:27:30.000Z",
      "key": 1566397650000,
      "doc_count": 3,
      "metric": {
        "value": null
      },
      "metric_deriv": {
        "value": null,
        "normalized_value": null
      }
    }
  ]
}

The key part of this is, under each named aggregation, the structure is identical (or rather consistent) in that it has like metric and metric_deriv. Or, in some cases like node_cgroup_quota, there are extra aggregated metrics like usage and periods. Either way, in the response, the structure is normalized under the named aggregation into a consistent format.

This consistent format is expected in other areas of the code. I'll use the node_cgroup_quota example again. Here is the code that reads one of those normalized metric aggregation stuctures.

Some other examples of shared code expecting this normalized structure:

• https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/monitoring/server/lib/details/get_series.js#L190
• https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/monitoring/server/lib/metrics/elasticsearch/classes.js#L124

Now, let's look how this changes with a top level date_histogram aggregation.

That might look like (again, I removed redundant parts):

{
  "aggs": {
    "nodes": {
      "terms": {
        "field": "source_node.uuid",
        "size": 10000
      },
      "aggs": {
        "by_date": {
          "date_histogram": {
            "field": "timestamp",
            "min_doc_count": 1,
            "fixed_interval": "30s"
          },
          "aggs": {
            "node_cgroup_quota": {
              "usage": {
                "max": {
                  "field": "node_stats.os.cgroup.cpuacct.usage_nanos"
                }
              },
              "periods": {
                "max": {
                  "field": "node_stats.os.cgroup.cpu.stat.number_of_elapsed_periods"
                }
              },
              "quota": {
                "min": {
                  "field": "node_stats.os.cgroup.cpu.cfs_quota_micros"
                }
              },
              "usage_deriv": {
                "derivative": {
                  "buckets_path": "usage",
                  "gap_policy": "skip",
                  "unit": "1s"
                }
              },
              "periods_deriv": {
                "derivative": {
                  "buckets_path": "periods",
                  "gap_policy": "skip",
                  "unit": "1s"
                }
              }
            },
            "node_cgroup_throttled": {
              "metric": {
                "max": {
                  "field": "node_stats.os.cgroup.cpu.stat.time_throttled_nanos"
                }
              },
              "metric_deriv": {
                "derivative": {
                  "buckets_path": "metric",
                  "unit": "1s"
                }
              }
            }
          }
        }
      }
    }
  }
}

This won't work because our named aggregation doesn't actually have a sub aggregation anymore (it used to be date_histogram) so we'd need to remove those entirely, which would make our structure look like (this is NOT shortened):

{
  "aggs": {
    "nodes": {
      "terms": {
        "field": "source_node.uuid",
        "size": 10000
      },
      "aggs": {
        "by_date": {
          "date_histogram": {
            "field": "timestamp",
            "min_doc_count": 1,
            "fixed_interval": "30s"
          },
          "aggs": {
            "usage": {
              "max": {
                "field": "node_stats.os.cgroup.cpuacct.usage_nanos"
              }
            },
            "periods": {
              "max": {
                "field": "node_stats.os.cgroup.cpu.stat.number_of_elapsed_periods"
              }
            },
            "quota": {
              "min": {
                "field": "node_stats.os.cgroup.cpu.cfs_quota_micros"
              }
            },
            "usage_deriv": {
              "derivative": {
                "buckets_path": "usage",
                "gap_policy": "skip",
                "unit": "1s"
              }
            },
            "periods_deriv": {
              "derivative": {
                "buckets_path": "periods",
                "gap_policy": "skip",
                "unit": "1s"
              }
            },
            "metric": {
              "max": {
                "field": "node_stats.fs.total.available_in_bytes"
              }
            },
            "metric_deriv": {
              "derivative": {
                "buckets_path": "metric",
                "unit": "1s"
              }
            }
          }
        }
      }
    }
  }
}

Even though we are supposed to have 6 named aggregations, we only see one metric and one metric_deriv because we can't have multiple of the same named key in a JSON structure.

This is the main issue.

We need to convert the structure to maintain the named aggregation, but rather than having it in a higher aggregation, we need to prefix the metric aggregation named with the named aggregation, something like node_cgroup_quota_periods.

Of course, once we do that, we need to undo that structure, as the rest of the shared code (linked earlier) would not know to look for that, or even care about the prefix naming.

I'm all for a better approach if anyone has any thoughts, but I didn't see a path forward outside of what I did in the PR.

[cachedout]

Thanks for the explanation.

I thought about this and I can't see a better way given the way the application is structured right now. Another approach seems like it might be to back away from trying to fetch a set of metrics for a set of nodes in a single ES request and instead break that up into multiple smaller requests but I'm pretty wary about going down that road, especially given that it's hard to believe that wouldn't be dramatically less performant for this case.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[igoristic]

Awesome stuff @chrisronline! This fix alone will have a great impact on performance

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[chrisronline]

@cachedout I think this should be all ready for you to review again when you get the chance

[cachedout]

I think this is good. 👍 on the thorough documentation and explanation in the comments.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[chrisronline]

retest

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[chrisronline]

Backport:

7.x: d4f0efc
7.3: 87ec6ed
6.8: c814843

[cachedout]

As a related side-note, the ES team is tracking a severe performance regression in date_histogram in 7.3.

[chrisronline]

@cachedout Nice find! I'm not quite sure how this information relates to this PR though. Do you mind providing more more details about the connection?

[cachedout]

I only put it here because it was one of the places where we've been working with date_histogram aggs recently and I figured that if somebody searched for "monitoring" and date_histogram they'd come across this issue.