-
Notifications
You must be signed in to change notification settings - Fork 1.2k
(feat): Add Sumo Logic scaler #6736
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
7074035
to
f802504
Compare
f1222ad
to
3a0e20b
Compare
e31fa20
to
b1d44bd
Compare
My feedback will focus on the possibility of using TypedConfig. This scaler does not use the new TypedConfig pattern available in the scalersconfig package. The metadata is manually parsed in parseSumoMetadata instead of declaratively with tags.
Also there is a some of manual validation logic that can be handled automatically with TypedConfig. Furthermore, I would keep GetMetricsAndActivity cleaner like the other scalers, like this:
Further, I am very curious to see how the Multi-Metrics Query Trigger is viewed by the community/maintainers (because of this discussion) |
Thanks for reviewing! I'll try out the things you mentioned above.
I see, it's a bit similar but the use-case is different. In Sumo Logic's case, that is how you write chained queries. Unlike prometheus where you can just let's say divide two metrics to calculate % utilisation or something, in Sumo Logic you have to define those metrics in separate queries and write another query which can reference those queries to do the chained calculation. The backend does all the calculation and we just pick result of the chained query via the input resultQueryRowID and ignore the rest. For example -
Here, we are just interested in the result of |
Done! |
Great! Looks good! Just curious, in the |
You are right, it's not needed. Removed it. Thanks! |
/run-e2e sumologic |
/run-e2e sumologic |
/run-e2e sumologic |
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.
Pull Request Overview
Adds a new Sumo Logic scaler to KEDA for logs- and metrics-based autoscaling, including implementation, registration, and tests.
- Introduce Sumo Logic scaler under
pkg/scalers
and register in the builder - Add unit tests for metadata parsing/metric spec and an e2e test
- Update test environment variables and document the new scaler in CHANGELOG
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/scalers/sumologic/sumologic_test.go | Add end-to-end test for Sumo Logic scaler |
tests/.env | Add Sumo Logic environment variables |
pkg/scaling/scalers_builder.go | Register the sumologic scaler |
pkg/scalers/sumologic_scaler_test.go | Unit tests for metadata parsing and metric naming |
pkg/scalers/sumologic_scaler.go | Core Sumo Logic scaler implementation |
pkg/scalers/sumologic/types.go | HTTP client and type definitions for Sumo Logic API |
pkg/scalers/sumologic/sumologic.go | API logic for log and metrics queries |
pkg/scalers/sumologic/query.go | Query builder for Sumo Logic API |
pkg/scalers/sumologic/metrics.go | Metrics query creation and response parsing |
pkg/scalers/sumologic/logs.go | Logs search, paging, and processing |
CHANGELOG.md | Document new Sumo Logic scaler |
Comments suppressed due to low confidence (4)
tests/scalers/sumologic/sumologic_test.go:200
- The assertion message says "after 3 minutes" but the timeout is set to 60 seconds; update the message or adjust the timeout to match.
"replica count should be %d after 3 minutes", maxReplicaCount)
tests/scalers/sumologic/sumologic_test.go:211
- The assertion message says "after 3 minutes" but the timeout is set to 60 seconds; either extend the timeout or correct the message.
"replica count should be %d after 3 minutes", minReplicaCount)
tests/scalers/sumologic/sumologic_test.go:216
- Use t.Skip or t.Skipf when SUMO_LOGIC_COLLECTOR_URL is not set to explicitly skip the data-push loop, preventing silent test passes with no data pushed.
t.Logf("No collector URL set, failed data push to Sumo Logic")
tests/scalers/sumologic/sumologic_test.go:161
- [nitpick] Add checks at the start of this test to verify SUMO_LOGIC_ACCESS_ID, SUMO_LOGIC_ACCESS_KEY, and SUMO_LOGIC_COLLECTOR_URL are set and call t.Skipf if any are missing to avoid misleading failures.
func TestSumologicScaler(t *testing.T) {
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, thank you!
ptal @JorTurFer, @zroubalik |
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
Signed-off-by: mittalvaibhav1 <[email protected]>
/run-e2e sumologic |
@JorTurFer, @zroubalik, given this has approval from @rickbrouwer and myself, I am going to merge this so we have time to watch the e2es and debug until 2.18 release. In the worst case, we can always revert. |
It looks like the e2e test is failing on the main. Is that something that needs to be looked into? |
@rickbrouwer Ack, looking into it. |
Looks like the search job API isn't allowed for this free account type. Can we mute this E2E or revert the PR until I can sort this out internally? |
I think we can wait a little longer until you have figured it out. As @wozniakjan earlier said :
so, keep us updated :) |
Thank you! I should have a response by Monday. |
/run-e2e sumologic |
I missed this earlier, but looks like the sumologic e2e test even though it passed, it had issues even before, can you please take a look at that too @mittalvaibhav1?
|
Yeah, also in the latest run I see those logs: https://github.com/kedacore/keda/actions/runs/16264440131/job/45917864210#step:9:569 |
It's a logical error when query results in no data. This should fix it |
Add Sumo Logic scaler to scale based and logs and metrics from Sumo Logic
Checklist