Fix metrics injection #3315
Fix metrics injection #3315
Conversation
License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
Kubuxu
force-pushed
the
fix/metrics-injection
branch
2 times, most recently
from
c56e4a4
to
7425a21
Compare
License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
Kubuxu
force-pushed
the
fix/metrics-injection
branch
from
7425a21
to
e254f0d
Compare
|
|
||
| err := mprome.Inject() | ||
| if err != nil { | ||
| log.Warningf("Injecting prometheus handler for metrics failed with message: %s\n", err.Error()) |
There was a problem hiding this comment.
should this just be a warning? Or should we actually error out here?
Also, can we (in a future PR) have a flag to disable metrics entirely?
There was a problem hiding this comment.
Yes we can have flag, lacking metrics won't hurt working of the daemon so IMO it is better to continue.
If you want I can do Error log here instead.
There was a problem hiding this comment.
Disabling of the metrics would be just not injecting the metrics.
License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
|
|
||
| err := mprome.Inject() | ||
| if err != nil { | ||
| log.Errorf("Injecting prometheus handler for metrics failed with message: %s\n", err.Error()) |
There was a problem hiding this comment.
How will :5001/debug/metrics/prometheus behave if Inject() fails?
There was a problem hiding this comment.
It will miss the metrics that are being added with metrics-interface and that is it.
Without this patch it runs as if the Inject has failed.
ghost
mentioned this pull request
I made last minute change last time and didn't test it properly.
This disabled metrics-interface as injection was being done after all collectors were being registered.