-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
📈 PaaS-friendly metrics #4874
📈 PaaS-friendly metrics #4874
Conversation
server.js
Outdated
const server = (module.exports = new Server(config)) | ||
const server = (module.exports = new Server(config, { | ||
id: process.env.INSTANCE_ID, | ||
env: process.env.INSTANCE_ENV || process.env.NODE_CONFIG_ENV || 'unknown', |
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.
@paulmelnikow would you like to handle env
similarly to instanceIdFrom
? I think we can simply use https://github.com/badges/shields/blob/master/config/custom-environment-variables.yml for reading env
.
If we want to remove env
from the piece of code above, I suggest to remove instance metadata object.
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.
Hmm, to keep things simple I'd suggest we do one of these two:
- Always use NODE_CONFIG_ENV
- Specify the influx env in config as a string literal, and rely on setting it using the env var set in custom_environment_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.
I chose option 2, because this will work for review apps - we want to set the env value to the PR review application name.
@paulmelnikow I think I fixed all issues. I will also update PR description. I would like to enable dyno metadata in our staging/review apps. Is it OK for you that I will do it? https://devcenter.heroku.com/articles/dyno-metadata#usage |
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.
Let's get it merged!
We probably need to add the production influx config, though that could happen in a follow-on PR.
@paulmelnikow and @chris48s, thank you for the review 👍 |
Glad to see this landed! Well done everyone 🎉 |
@platan Do you want to send me some credentials for the production server through Keybase? Or should I snag them from staging? |
I created a separate user for the production. I send you credentials through Keybase. |
This PR tries to fix #3946 :-)
Current metrics
/metrics
resource, e.g. https://s0.servers.shields.io/metrics (the server is using self-signed SSL certificates)Problem we want to solve
After moving to Heroku (or possibly other PaaS), we are not able to access metrics (
/metrics
endpoint) from specific instance.Solution
Solution was proposed in this comment
Why converting to Influx format?
Currently Telegraf cannot accept metrics in Prometheus format in HTTP listener, but there are plans to add such feature.
How to identify instances?
Currently metrics are identified by 2 labels (can be observed at https://metrics.shields.io/d/g_1B7zhik/prom-client-default-metrics):
Both labels are added to metrics by Prometheus, metrics in
/metrics
endpoint do not contain such information.Now we have add information described above to metrics.
The
instance
value source can be set usingpublic.metrics.influx.instanceIdFrom
configuration property. 3 values are allowed:env-var
- a value will be read from an environment variable with a name defined by thepublic.metrics.influx.instanceIdEnvVarName
configuration propertyhostname
- a value will be equal to https://nodejs.org/api/os.html#os_os_hostname`. This value can be mapped usinghostnameAliases
random
, e.g.fyytvm7el
The
env
value can be defined usingpublic.metrics.influx.envLabel
configuration property.There is also a new configuration property
public.metrics.prometheus.endpointEnabled
, which allows to enable/disable the/metrics
endpoint.production (VPS at OVH)
We can leave current metrics as they are. But we can also use the new approach. This configuration should enable new metrics and disable the old one:
+
INFLUX_USERNAME
andINFLUX_PASSWORD
environment variables.The code below shows what
os.hostname()
returns at OVH metrics server:I guess our servers should return vps71670, vps244529 and vps117870 for
os.hostname()
(https://github.com/badges/shields/blob/master/doc/production-hosting.md#badge-servers)Heroku
I suggest to use https://devcenter.heroku.com/articles/dyno-metadata#dyno-metadata to identify instances.
HEROKU_DYNO_ID can be used as an instance id.
If we want to use Dyno metadata, we have to enable it.
In the staging app(https://shields-staging.herokuapp.com) and in review apps we can use app name (
HEROKU_APP_NAME
) as an env value. This will give valuesshields-staging
andshields-staging-pr-XXXX
. We can send metrics from all env (production, staging, review apps) to http://metrics.shields.io/.Telegraf is already running at http://metrics.shields.io/ and at http://metrics-test.shields.platan.space (a separate instance I use for testing). Configuration can be found here. I already send some metric from my locally running app to https://metrics-test.shields.platan.space/d/g_1B7zhik/prom-client-default-metrics?orgId=1&from=1586112499614&to=1586113011638&var-env=development&var-instance=rpy2hz7cd
Follow ups: