feat(metrics): histogram metric for loop latency #5812
Conversation
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. |
Signed-off-by: Hy3n4 <[email protected]>
Signed-off-by: Hy3n4 <[email protected]>
Hy3n4
force-pushed
the
feature/internal_loop_histogram_metrics
branch
from
99460e2
to
43e362a
Compare
|
Hey, I would like to revive this PR. I've found some time to test it out and it does exactly what I need from it to do. At least I think it does. I would appreciate another review and possibly the direction what else should I do to have this PR ready for merge 🙏 . |
|
Sure, I'll try my best. |
keda_internal_scale_loop_latency_seconds_bucket Signed-off-by: Hy3n4 <[email protected]>
Signed-off-by: Hy3n4 <[email protected]>
Signed-off-by: Hy3n4 <[email protected]>
|
/run-e2e metric |
|
Could you update docs as well to include this metric? |
Signed-off-by: Hy3n4 <[email protected]>
|
It's done. Although I'm not sure I chose the right place for it 😄 |
Could you send me the link? I can't find it :( |
Signed-off-by: Hy3n4 <[email protected]>
Never mind, I mixed up docs and changelog. So I changed the changelog. I'll revert it and edit the docs. Should I add it to version |
| @@ -904,6 +904,37 @@ func testScalableObjectMetrics(t *testing.T) { | |||
| } | |||
| } | |||
| assert.Equal(t, true, found) | |||
|
|
|||
| val, ok = family["keda_internal_scale_loop_latency_seconds_bucket"] | |||
There was a problem hiding this comment.
I'm not 100% sure, but I think that the name is keda_internal_scale_loop_latency_bucket here
e2e test has failed here
There was a problem hiding this comment.
Hmm, I see the problem with otel. There is no otel metric for this. The problem is I don't have enough expertise to create an otel histogram metric 🤔 So we have two options. Remove e2e test or make the otel metric somehow.
There was a problem hiding this comment.
I'd say that we should include it also for otel as it has support for histograms:
https://github.com/open-telemetry/opentelemetry-go/blob/fe6c67e7e9b408d7f1e05356e4ccafadf0475b10/example/prometheus/main.go#L69-L80
There was a problem hiding this comment.
you have to call the the record function instead of using an observable approach
There was a problem hiding this comment.
Aha, now I can see in the code that I tried something, but apparently it's not the right way to do it.
This PR is supposed to add new histogram type of metric for the internal loop latency. This should be more useful when creating the SLIs.
Checklist