-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: Introduce operational metrics in OpenTelemetry Collector #4860
feat: Introduce operational metrics in OpenTelemetry Collector #4860
Conversation
Signed-off-by: SpiritZhou <[email protected]>
Thank you for your contribution! 🙏 We will review your PR as soon as possible.
While you are waiting, make sure to:
Learn more about: |
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an awesome improvement!! 💪
I have left some comments inline, but apart from them:
As testing otel is a must, I'd move the Kustomization to config/e2e because it's the Kustomization file that we use for deploying e2e tests (for releases we use config/default). It makes easier to maintain the files and also ensures that always is deployed
We also have to document this new option in keda-docs 🙏
We also need to expose this in our Helm chart but that's a separate PR as well |
Co-authored-by: Tom Kerkhove <[email protected]> Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
/run-e2e sequential |
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
+1 |
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
@zroubalik @JorTurFer Just updated the code. Hope it can meet the ddl. |
@SpiritZhou thanks! I am on it now, do you think you can update Helm charts as well? |
/run-e2e sequential |
/run-e2e sequential |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, prometheus tests is failing:
prometheus_metrics_test.go:336: --- testing scaled object errors ---
helper.go:567: Deleting template: scaledObjectTemplate
helper.go:5***9: Applying template: wrongScaledObjectTemplate
prometheus_metrics_test.go:35***:
Error Trace: /__w/keda/keda/tests/sequential/prometheus_metrics/prometheus_metrics_test.go:35***
/__w/keda/keda/tests/sequential/prometheus_metrics/prometheus_metrics_test.go:***67
Error: Should not be: 0
Test: TestPrometheusMetrics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SpiritZhou one more thing, could you please make it an experimental feature? Marking it so in the docs and changelog?
There's nothing wrong with the feature itself. Just for us to be able to change the metrics or nomenclature if needed without the need to keep backwards compatibility.
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
@zroubalik I fixed the test and update the "experimental feature". Could you help to review them again? |
/run-e2e sequential |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the quick fix!
Talked to @SpiritZhou and he'll start Helm chart PR, if he can't finish it today I'll take over tomorrow. That should allow us to merge this one in time |
It is strange. The prometheus_metrics_test can be passed in my local minikube env... |
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
/run-e2e sequential |
Signed-off-by: SpiritZhou <[email protected]>
@zroubalik @JorTurFer @tomkerkhove The failed e2e test may not be caused by OpenTelemetry PR. I found it happened in this PR before (https://github.com/kedacore/keda/actions/runs/6308971553). And I also found that the correct metric value can be retrieved after around 15s. I don't have any idea why it becomes slow. |
Right, it is failing on |
Signed-off-by: SpiritZhou <[email protected]>
I will pick up Helm chart later today |
…ore#4860) Co-authored-by: Tom Kerkhove <[email protected]> Signed-off-by: anton.lysina <[email protected]>
Refactor the current Prometheus.go file by adding a middleware metriccollector.go to handle initialization and metrics collection for both Prometheus and OpenTelemetry.
About e2e test:
Checklist
Relates to #3078
Relates to kedacore/keda-docs#1215