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

Enabled the selection of cloudflare speedtest #475

Closed

Conversation

martinbrose
Copy link
Contributor

Update to closed pull request #387.

Created a new Docker image called redorbluepill/cloudflare-speedtest-exporter to replace the current Ookla Speedtest Prometheus Exporter and enabled the selection of the speedtest images through a variable called monitoring_speedtest_provider in config.yml.
Replacing the image in file templates/docker-compose.yml.j2 is implemented with an if statement based on the speedtest variable in the config file. The original speedtest based on Ookla will always be the default, in case of missing config variable or misspelling.

@geerlingguy, I think that addresses your last comment on closed pull request #387.

Fixes #341:

@Maxxxel
Copy link

Maxxxel commented Feb 6, 2023

Does this fix the Gigabit issues with speedtest-cli?

@stale
Copy link

stale bot commented May 21, 2023

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

Please read this blog post to see the reasons why I mark pull requests as stale.

@stale stale bot added the stale label May 21, 2023
@geerlingguy
Copy link
Owner

The only concern here is the maintenance of the Docker image — I'm willing to merge this in, as I do think a choice would be nice (and it may help with certain types of connections), but I also see the maintenance history of speedtest-exporter vs. cloudflare-speedtest-exporter, and just want to make sure I don't have to yank this change if the downstream image goes unmaintained and starts breaking.

@geerlingguy geerlingguy removed the stale label Jul 18, 2023
@martinbrose
Copy link
Contributor Author

Hi @geerlingguy, thanks for you reply.
I'm happy to maintain the underlying repo and Docker images.

I guess for most people the Ookla speedtest should be fine. I just had the use case that it wasn't working well for me.

BR

@martinbrose
Copy link
Contributor Author

Does this fix the Gigabit issues with speedtest-cli?

Hi @Maxxxel, I'm not sure which Gigabit issue you're talking about. Do you have an issue ID / reference?

@martinbrose
Copy link
Contributor Author

Hi @geerlingguy,

just a gentle ping on this PR.
I updated the Docker image to be significantly smaller than before by removing the Numpy dependency in the underlying Python cloudflarepycli dependency (martinbrose/cloudflarepycli#14).
I think the maintenance history is also a function of the actual use of this solution. So far it's probably just me and a few other users using my Docker file.

Tbh, I don't mind either way. If you decide not to merge it, it's fine with me too... at least people can find the closed PR and take this for a spin by merging the change themselves.

@Nytelife26
Copy link

interestingly, after making the switch over, the exporter outputs 2 decimal places to the log, but either Grafana or Prometheus seems to be truncating it. i am not sure why, but the graphs and gauge readings now only display integers.

this is likely not a result of your tooling, but i was wondering if you had any ideas.
in any case, it's nice to have alternatives. i am hoping this might get merged in.

@martinbrose
Copy link
Contributor Author

Hi @Nytelife26,

thanks for your sharing your observation. I'm currently on holidays, but will have a look on my return next week.

Cheers

@martinbrose
Copy link
Contributor Author

Hi @Nytelife26,

my apologies, left this hanging for a while but finally got around looking into it.
The problem was within my code base where I used int() at one point.

This should be solved with the latest commit and the new release https://github.com/martinbrose/cloudflare-speedtest-exporter/releases/v1.2.0.

Feel free to check this out with the latest container from Docker Hub or GHCR.

@Nytelife26
Copy link

my apologies, left this hanging for a while but finally got around looking into it.
The problem was within my code base where I used int() at one point.

This should be solved with the latest commit and the new release https://github.com/martinbrose/cloudflare-speedtest-exporter/releases/v1.2.0.

consider me sold, i'll be trialling your exporter full time and i'll let you know if anything else is up, but i think it should be a strong alternative. good work :)

Copy link

github-actions bot commented Jul 1, 2024

This pr has been marked 'stale' due to lack of recent activity. If there is no further activity, the issue will be closed in another 30 days. Thank you for your contribution!

Please read this blog post to see the reasons why I mark issues as stale.

@github-actions github-actions bot added the stale label Jul 1, 2024
@martinbrose martinbrose closed this Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quality of Life :: Enhancement Requests
4 participants