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

feature/metrics-naming #34

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Conversation

loelkes
Copy link
Contributor

@loelkes loelkes commented Nov 3, 2022

As discussed in #33

I'll follow up with some more refactoring and do some tests.

@loelkes loelkes changed the title Feature/metrics naming feature/metrics-naming Nov 3, 2022
@loelkes loelkes marked this pull request as draft November 4, 2022 08:02
@tg44
Copy link
Owner

tg44 commented Nov 11, 2022

LGTM!

@loelkes loelkes marked this pull request as ready for review January 6, 2024 15:36
@loelkes
Copy link
Contributor Author

loelkes commented Jan 6, 2024

This is a breaking change, so I recommend releasing a new major version.

@tg44
Copy link
Owner

tg44 commented Feb 1, 2024

Can you rebase this? There was two not so big changes that I merged today, and this branch was already conflicting.

@loelkes
Copy link
Contributor Author

loelkes commented Feb 2, 2024

Done.

I would like to discuss the timer reset after printing. In my opinion it should be removed.

  • It depends on your scrape interval.
  • Why reset the values at all? If something has been printed, cancelled, etc. the progress remains unchanged unless someone touches it.

This could also be achieved with G4 and G92 commands at the end/beginning of the print, with a dwell time to match your scrape interval. Right now there's no way to opt out of this. If you agree, I would open a new feature branch for this (including documentation).

@hyperair
Copy link
Contributor

hyperair commented Feb 5, 2024

I was playing with my Grafana dashbaord recently and found it quite annoying when the print progress didn't get reset properly -- it led to a ballooning of metrics, and was quite cumbersome to implement a simple ETA or Print time left gauge widget because now I had to filter out all the time series that weren't current. Clearing the print progress metrics at the end of each print ensures that there's just one relevant time series running at a time, which results in graphs like so:

image

The top graph has a properly reset metric, but the bottom graph, due to a bug fixed in #41, didn't get reset properly between the orange and yellow lines, causing them to overlap. This Octoprint session only had a single digit number of prints running, but it accumulates lines and ends up with an increasingly noisy graph unless you add special field overrides in the Grafana widget to filter out irrelevant series.

@tg44
Copy link
Owner

tg44 commented Feb 5, 2024

We need to reset the timers bcs they are starting to overlap if your instance is not reseting. In my opinion, we want to measure a print. A print starts when the label first appear, this is logical, we can't measure a print job that was never created/started in octoprint. But on the other hand we will finish a job at some point in time, so we need to "clear" that job when octoprint success/fail it.

When you have a single printer setup, it is logical that only one line is on the ETA, or the print progress graph. If you have lets say 5 printers, you want to see at most 5 lines on the graph at a time. If we do not reset the progress, than in a single printer setup, you will have N lines where N is the number of jobs ever started on that octoprint instance from its last restart.

It depends on your scrape interval.
Yes, if we would have a settings page, probably the "reset interval" should go there...

@tg44 tg44 mentioned this pull request Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants