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

Add metrics rendering to the new topology view. #8858

Merged
merged 29 commits into from
Oct 9, 2020

Conversation

banks
Copy link
Member

@banks banks commented Oct 7, 2020

This is mostly working but certainly has bugs and a few TODOs:

TODO

  • actually get the service's protocol from somewhere rather than hard code it
  • test against real Prometheus not just the mocks
  • handle loading states for metrics
  • if provider doesn't return any series the graph will just break instead of showing an empty state
  • Do something sensible if there is no metrics provider configured.

Known Issues

  • The graph svg is underneath the SVG for the arrows at either end for about 10 pixels which means the tooltip won't show when hovering either end of the graph.
    • Can be fixed with CSS pointer-events: none; on the arrow divs (assuming we don't need mouse interaction on them.
  • The stats don't fit in the boxes at all widths and probably need to flex more and/or dissappear when there is not enough room for them.
  • The graph doesn't redraw nicely when browser screen size changes. It does fix itself on the next poll/update though!
  • HTTP series need a re-think. We wanted to show error rates as a percentage which is nice but it actually means the stacking of the graphs is misleading since "percent" and whatever scale is appropriate for RPS are not the same thing but we render the two areas on the same scale. So "1%" error rate might visually look like 1 percent on the graph but only if the actual (max) request rate is exactly 100rps, if the max request rate is 1000 it will look like 0.1% etc. This means we should actually fetch the stat differently in the provider (easy and I actually did it that way first but then changed back to get the "percent", thinking it would be easy to render them on different scales but it's... not).

Dev notes.

There are some new cookie values you can set to control the topology data:

  • CONSUL_UPSTEAM_COUNT
  • CONSUL_DOWNSTREAM_COUNT
  • CONSUL_METRICS_POLL_INTERVAL (in milliseconds) this is useful to stop updating while you are trying to debug the stats/graph layout etc.
  • CONSUL_METRICS_LATENCY_MAX (in milliseconds) will add an artificial random delay up to this value to metrics loading to see the loading experience more like with a real backend. Can set it really long to style loading elements.

@mikemorris
Copy link
Contributor

mikemorris commented Oct 7, 2020

Working on getting this setup to test against real Prometheus in Firefox and getting 404s from the metrics proxy (but at least I've got it loading the module now!) - how would you pass config to tell it where to find a Prometheus instance in a setup like https://github.com/mkeeler/consul-docker-test/tree/master/mesh-gateways-l7?

@banks
Copy link
Member Author

banks commented Oct 8, 2020

@mikemorris sorry doc are on my TODO list here!

The correct config for this for a "real" prometheus should be:

ui_config {
  enabled = true
  metrics_provider = "prometheus"
  metrics_proxy {
    base_url = "http(s)://<your prometheus instance>:<port>"
  }
}

In Matt's docker setup, I believe prometheus is exported on localhost:9090 so it would be: base_url = "http://localhost:9090"

@banks
Copy link
Member Author

banks commented Oct 8, 2020

Last commit threaded protocol through based on the still-in-review #8868 and it's api-double counterpart: hashicorp/consul-api-double#39

@banks banks added this to the 1.9 milestone Oct 8, 2020
<div class="tooltip">
<div class="sparkline-time">Timestamp</div>
</div>
{{!-- TODO: fix loading state --}}
Copy link
Member Author

Choose a reason for hiding this comment

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

this is done. will remove

@banks banks force-pushed the ui/feature/topology-metrics branch from 7079854 to 2ab9f9e Compare October 8, 2020 17:17
@mikemorris
Copy link
Contributor

mikemorris commented Oct 9, 2020

Got this running against a live Prometheus instance and gonna drop a few notes here:

  • Upstreams and downstreams show metrics as expected, but selected service does not?

Screen Shot 2020-10-08 at 8 15 00 PM

Screen Shot 2020-10-08 at 8 15 16 PM

{"status":"success","data":{"resultType":"vector","result":[]}}
{"status":"success","data":{"resultType":"matrix","result":[]}}
{"status":"success","data":{"resultType":"matrix","result":[]}}

Screen Shot 2020-10-08 at 9 29 35 PM

Screen Shot 2020-10-08 at 9 29 03 PM

  • Some metrics only being visible at larger screen sizes was definitely unexpected, this might need some design work for a more responsive layout (stacked vertically or something maybe?)

banks and others added 18 commits October 9, 2020 20:01
 - Loading state for graph
 - Added fake latency cookie value to test loading
 - If metrics provider has no series/stats for the service show something that doesn't look broken
 - Graph hover works right to the edge now
 - Stats boxes now wrap so they are either shown or not as will fit not cut off
 - Graph resizes when browser window size changes
 - Some tweaks to number formats and stat metrics to make them more compact/useful
1. Remove fakeLatency setTimeout (will be replaced with CONSUL_LATENCY
in mocks)
2. Make sure any event handlers are removed
@banks banks marked this pull request as ready for review October 9, 2020 20:22
Copy link
Contributor

@kaxcode kaxcode left a comment

Choose a reason for hiding this comment

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

:shipit: ✨ 🎨 LGTM

@banks banks merged commit 27048a0 into master Oct 9, 2020
@banks banks deleted the ui/feature/topology-metrics branch October 9, 2020 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants