Enable multi instances metrics #108
Conversation
jansouza
commented
Dec 30, 2021
jansouza
commented
- Use python:3.10-slim
- Include Packet Loss metric (speedtest_packet_loss)
- Remove speedtest_server_id metric and Add server_id to the metric label
- Use logging for debug/info (SPEEDTEST_LOG_LEVEL)
- Add /health
|
Hey @jansouza really thanks for your contribution, whenever I can I will take a look on that! Thanks, |
There was a problem hiding this comment.
Hey.
@jansouza really thanks for you PR. I have made some comments in things that I think that should be changed. Sorry for the delay.
Thanks,
MiguelNdeCarvalho
Dockerfile
Outdated
| @@ -1,29 +1,30 @@ | |||
| FROM python:3.10.1-alpine3.15 | |||
| FROM python:3.10-slim as build | |||
There was a problem hiding this comment.
Why do we need to change the base image?
|
|
||
| WORKDIR /app | ||
| RUN python -m venv /app/venv |
There was a problem hiding this comment.
Is there a specific way to use a venv inside the container?
| #!/bin/bash | ||
| set -e | ||
|
|
||
| docker build -t speedtest-img . |
There was a problem hiding this comment.
Why do we need this build.sh as all builds are done by CI
| @@ -0,0 +1,8 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Why do we need this destroy.sh script too?
There was a problem hiding this comment.
since hes not using docker-compose. hes building with "build.sh" and destroying with "destroy.sh"
- stop both servers
- remove both containers
- remove the image
| @@ -0,0 +1,5 @@ | |||
| [flake8] | |||
There was a problem hiding this comment.
Any specific reason for ignoring this?
| @app.route("/") | ||
| def mainPage(): | ||
| return ("<h1>Welcome to Speedtest-Exporter.</h1>" + | ||
| "Click <a href='/metrics'>here</a> to see metrics.") | ||
| "Click <a href='/metrics'>here</a> to see metrics.<br>" + | ||
| "Click <a href='/health'>here</a> to see healthy.") |
There was a problem hiding this comment.
There is no need to add the /health path here in my opinion!
