-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: e2e test checks label value by name and not by index #6858
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
Conversation
Signed-off-by: Jorge Turrado <[email protected]>
|
/run-e2e sequential |
Signed-off-by: Jorge Turrado <[email protected]>
|
/run-e2e sequential |
Signed-off-by: Jorge Turrado <[email protected]>
|
/run-e2e sequential |
dttung2905
left a comment
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.
I think it is a cleaner way of accessing the Prom label value. The only downside is a slight degrade in performance. For each of the label, we are looping through the whole arr which is O(n) instead of the current way of using index with O(1) time complexity. I personally think it is acceptable to trade performance for readability & maintainability in this case.
If you have a better way, let me know too 😃
yeah, I did this change for e2e because it's just a single place to use and actually, the old approach failed on upstream changes, but definitively it'd not be good for production :) |
) * fix: e2e test checks label value by name and not by index Signed-off-by: Jorge Turrado <[email protected]> * fix: e2e test checks label value by name and not by index Signed-off-by: Jorge Turrado <[email protected]> * fix: e2e test checks label value by name and not by index Signed-off-by: Jorge Turrado <[email protected]> --------- Signed-off-by: Jorge Turrado <[email protected]> Signed-off-by: kmoonwright <[email protected]>
) * fix: e2e test checks label value by name and not by index Signed-off-by: Jorge Turrado <[email protected]> * fix: e2e test checks label value by name and not by index Signed-off-by: Jorge Turrado <[email protected]> * fix: e2e test checks label value by name and not by index Signed-off-by: Jorge Turrado <[email protected]> --------- Signed-off-by: Jorge Turrado <[email protected]> Signed-off-by: kmoonwright <[email protected]>
) * fix: e2e test checks label value by name and not by index Signed-off-by: Jorge Turrado <[email protected]> * fix: e2e test checks label value by name and not by index Signed-off-by: Jorge Turrado <[email protected]> * fix: e2e test checks label value by name and not by index Signed-off-by: Jorge Turrado <[email protected]> --------- Signed-off-by: Jorge Turrado <[email protected]> Signed-off-by: David Pochopsky <[email protected]>
As we don't check otel metrics directly but exported via Prometheus in collector, we are affected by posible changes in how the labels are generated, for example, with the collector v1.128.0, the metrics is:
keda_cloudeventsource_events_emitted_count_total{ cloudEventSource="opentelemetry-metrics-test-ce", eventsink="http", job="unknown_service:keda", namespace="opentelemetry-metrics-test-ns", otel_scope_name="keda-open-telemetry-metrics", otel_scope_schema_url="", otel_scope_version="", state="emitted" } 1And the same metric from the collector v1.127.0 is:
keda_cloudeventsource_events_emitted_count_total{ cloudEventSource="opentelemetry-metrics-test-ce", eventsink="http", job="unknown_service:keda", namespace="opentelemetry-metrics-test-ns", state="emitted" } 1This PR changes the search to get the value by key and not by position to be resilient to changes in the labels because of metric tranformations under the scope of the test
Checklist