Add per-badge metrics for BaseService #3093
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
paulmelnikow
changed the title
Merged
paulmelnikow
changed the title
|
@platan Would you be able to help me test this with a Prometheus instance? |
Just deploy your change somewhere, give me an address and I will add it to http://metrics.shields.io/ or https://metrics-test.shields.platan.space. Alternatively, you can test it locally using https://github.com/platan/metrics-shields-io-config (I'm still working on this, but it should be useful :-), just change configuration https://github.com/platan/metrics-shields-io-config/blob/feceb7fbeb15ed63d32e88d013ff6690131081df/shields-io-metrics.yml#L29) |
|
Labes is a powerful feature of Prometheus. we can have one metric with different labels: Every badge has a corresponding metric for total number of requests to this badge with labels:
This allows us to aggregate metrics using those labels. |
|
Let me take another pass through and use that! |
|
Here's what I've got now: Pretty cool! |
core/base-service/base.js
Outdated
| const emojic = require('emojic') | ||
| const Joi = require('joi') | ||
| // Ideally `validateMetricName()` would be in the public interface. | ||
| // https://github.com/siimon/prom-client/pull/246 |
There was a problem hiding this comment.
Looks like your PR has already been approved so hopefully that will get merged in before too long 👍
There was a problem hiding this comment.
Though… i've just removed it 😀
|
I've added |
|
I've added https://shields-staging-pr-3093.herokuapp.com/metrics to https://metrics-test.shields.platan.space/prometheus/graph (username:password: prometheus:prometheus). Sample graph: https://metrics-test.shields.platan.space/prometheus/graph?g0.range_input=1h&g0.expr=service_requests_total&g0.tab=0 |
paulmelnikow
changed the title
|
@platan How often does it poll? It looks like some metrics are getting dropped when the dyno gets re-deployed. |
|
Here's a graph that uses |
|
Metrics are pooled (scraped) every 15 seconds: https://metrics-test.shields.platan.space/prometheus/config, https://metrics-test.shields.platan.space/prometheus/targets.
|
|
I left a few minor questions/feedback items as well 👍 |
|
Using: METRICS_PROMETHEUS_ENABLED=true npm start
SKIP_INTERCEPTED=TRUE npm run test:services --
curl http://localhost:8080/metrics -s | grep "service_requests_total{" | sortI've gathered metrics for all(?) services: metrics |
platan
reviewed
Feb 26, 2019
|
Thanks for the reviews! I've updated this. |
8 tasks
calebcartwright
approved these changes
Feb 27, 2019
|
It's interesting to see in those results that there's quite a few requests for deprecated badges still coming through (gemnasium for example) |
|
Yea, for sure! And people actually using the maintenance badge. And lots of other interesting bits. I just said something a bit more general about it here: #966 (comment). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This picks up #2068 by adding per-badge stats as discussed in #966.
It ensures every service has a unique
nameproperty. By default this comes from the class name, and is overridden in all the various places where the class names are duplicated. (Some of those don't seem that useful, like the various download interval services, though those need to be refactored down into a single service anyway.) Tests enforce the names are unique. These are the names used by the service-test runner, so it's a good idea to make them unique anyway. (It was sort of strange before that you had to specifynugetinstead of e.g.resharper.)I've added validation to
deprecatedServiceandredirector, and required that everyroutehas abase, even if it's an empty string.The name is used to generate a unique metric name, which causes metrics to be generated like these:
This is predicated on being able to use Prometheus's
rate()function to visualize a counter's rate of change, as mentioned at #2068 (comment). Otherwise the stats will be disrupted every time a server restarts.The metrics are only used on new-style services, but I'm okay with that. They'll be ported soon enough.