Skip to content

ROX-28941: Use GetByQueryFn in alerts#15085

Merged
janisz merged 7 commits intomasterfrom
ROX-28941_use_getbyqueryfn_in_alerts
May 8, 2025
Merged

ROX-28941: Use GetByQueryFn in alerts#15085
janisz merged 7 commits intomasterfrom
ROX-28941_use_getbyqueryfn_in_alerts

Conversation

@janisz
Copy link
Contributor

@janisz janisz commented Apr 25, 2025 •
edited
Loading

This PR uses a feature created in #15010 to improve alerts service performance and present how to use GetByQueryFn in other places.

                                          │   old.txt   │              new.txt               │
                                          │   sec/op    │   sec/op     vs base               │
AlertDatabaseOps/searchWithRawListAlert-8   9.708m ± 1%   9.448m ± 1%  -2.68% (p=0.000 n=20)

                                          │   old.txt    │               new.txt               │
                                          │     B/op     │     B/op      vs base               │
AlertDatabaseOps/searchWithRawListAlert-8   8.334Mi ± 0%   8.327Mi ± 0%  -0.09% (p=0.000 n=20)

                                          │   old.txt   │              new.txt               │
                                          │  allocs/op  │  allocs/op   vs base               │
AlertDatabaseOps/searchWithRawListAlert-8   109.2k ± 0%   109.2k ± 0%  +0.00% (p=0.000 n=20)

More bench tests are in:

@janisz janisz requested review from a team as code owners April 25, 2025 13:41
@janisz janisz changed the base branch from master to ROX-28939_create_getbyqueryfn April 25, 2025 13:41
@janisz janisz removed the request for review from a team April 25, 2025 13:41
@janisz janisz added the ci-performance-tests Runs the K6 based performance tests label Apr 25, 2025
sourcery-ai[bot]
sourcery-ai bot reviewed Apr 25, 2025
View reviewed changes
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We encountered an error and are unable to review this PR. We have been notified and are working to fix it.

You can try again by commenting this pull request with @sourcery-ai review, or contact us for help.

@rhacs-bot
Copy link
Contributor

rhacs-bot commented Apr 25, 2025 •
edited
Loading

Images are ready for the commit at c5567f1.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-622-gc5567f135e.

@janisz janisz removed the request for review from a team April 28, 2025 10:27
@codecov
Copy link

codecov bot commented Apr 28, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 27.58621% with 21 lines in your changes missing coverage. Please review.

Project coverage is 49.08%. Comparing base (5f9fa69) to head (c5567f1).
Report is 34 commits behind head on master.

Files with missing lines Patch % Lines
...l/alert/datastore/internal/search/searcher_impl.go 0.00% 17 Missing ⚠️
central/platform/reprocessor/reprocessor_impl.go 63.63% 3 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #15085      +/-   ##
==========================================
- Coverage   49.11%   49.08%   -0.03%     
==========================================
  Files        2557     2557              
  Lines      187880   187832      -48     
==========================================
- Hits        92269    92192      -77     
- Misses      88339    88370      +31     
+ Partials     7272     7270       -2
Flag Coverage Δ
go-unit-tests 49.08% <27.58%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Base automatically changed from ROX-28939_create_getbyqueryfn to master April 30, 2025 17:03
@janisz janisz force-pushed the ROX-28941_use_getbyqueryfn_in_alerts branch from 9249caf to 4570267 Compare April 30, 2025 18:18
@openshift-ci
Copy link

openshift-ci bot commented Apr 30, 2025 •
edited
Loading

@janisz: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/ocp-4-18-compliance-e2e-tests 9249caf link false /test ocp-4-18-compliance-e2e-tests
ci/prow/ocp-4-18-qa-e2e-tests 9249caf link false /test ocp-4-18-qa-e2e-tests
ci/prow/ocp-4-18-operator-e2e-tests 9249caf link false /test ocp-4-18-operator-e2e-tests
ci/prow/ocp-4-12-operator-e2e-tests 9249caf link false /test ocp-4-12-operator-e2e-tests
ci/prow/ocp-4-12-compliance-e2e-tests 9249caf link false /test ocp-4-12-compliance-e2e-tests
ci/prow/ocp-4-12-qa-e2e-tests 9249caf link false /test ocp-4-12-qa-e2e-tests
ci/prow/gke-operator-e2e-tests 9249caf link false /test gke-operator-e2e-tests

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 30, 2025 •
edited
Loading

Performance test results

Summary

  █ main dashboard

  █ list alerts grpc

    ✓ status is OK

  checks.........................: 100.00% ✓ 50       ✗ 0  
  data_received..................: 3.7 MB  53 kB/s
  data_sent......................: 239 kB  3.4 kB/s
  group_duration.................: avg=621.99ms min=128.62ms med=594.58ms max=1.41s    p(90)=1.18s    p(95)=1.2s    
  grpc_req_duration..............: avg=135.74ms min=127.13ms med=134.17ms max=171.83ms p(90)=140.24ms p(95)=146.64ms
  http_req_blocked...............: avg=243.9µs  min=320ns    med=461ns    max=109.53ms p(90)=681ns    p(95)=762ns   
  http_req_connecting............: avg=120.8µs  min=0s       med=0s       max=54.36ms  p(90)=0s       p(95)=0s      
  http_req_duration..............: avg=122.59ms min=54.14ms  med=61.18ms  max=657.29ms p(90)=469.85ms p(95)=511.49ms
    { expected_response:true }...: avg=142.02ms min=58.08ms  med=62.6ms   max=657.29ms p(90)=482.25ms p(95)=529.29ms
  ✓ { lib:true }.................: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
  http_req_failed................: 22.22%  ✓ 100      ✗ 350
  http_req_receiving.............: avg=6.45ms   min=22.85µs  med=55.66µs  max=107.56ms p(90)=53.74ms  p(95)=54ms    
  http_req_sending...............: avg=89.85µs  min=39.42µs  med=90.7µs   max=262.54µs p(90)=116.99µs p(95)=127.89µs
  http_req_tls_handshaking.......: avg=122.08µs min=0s       med=0s       max=54.93ms  p(90)=0s       p(95)=0s      
  http_req_waiting...............: avg=116.04ms min=53.96ms  med=61ms     max=657.1ms  p(90)=469.67ms p(95)=511.32ms
  http_reqs......................: 450     6.428398/s
  iteration_duration.............: avg=1.4s     min=1.3s     med=1.37s    max=1.7s     p(90)=1.5s     p(95)=1.52s   
  iterations.....................: 50      0.714266/s
  vus............................: 1       min=1      max=1
  vus_max........................: 1       min=1      max=1

Sources

@janisz
Copy link
Contributor Author

janisz commented May 5, 2025 •
edited
Loading 

-grpc_req_duration..............: avg=86.26ms  min=83.74ms med=86.34ms max=90.55ms  p(90)=88.26ms  p(95)=89.34ms 
+grpc_req_duration..............: avg=75.89ms  min=72.33ms med=75.5ms  max=81.65ms  p(90)=80.22ms  p(95)=80.89ms

janisz added 6 commits May 5, 2025 11:04
@janisz
ROX-28941: use GetByQueryFn
0d35063 
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
@janisz
mocks
d08b748 
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
@janisz
fix
7a9048e 
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
@janisz
fix
0fb5f6b 
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
@janisz
fix
1dea53e 
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
@janisz
when unlimited
17ae1a1 
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
@janisz janisz force-pushed the ROX-28941_use_getbyqueryfn_in_alerts branch from 4570267 to 17ae1a1 Compare May 5, 2025 09:09
@janisz janisz requested review from mtodor and rhybrillou May 5, 2025 09:47
rhybrillou
rhybrillou approved these changes May 5, 2025
View reviewed changes
@janisz
extract common code
c5567f1 
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
@janisz janisz merged commit b364057 into master May 8, 2025
84 of 86 checks passed
@janisz janisz deleted the ROX-28941_use_getbyqueryfn_in_alerts branch May 8, 2025 07:50
@janisz janisz mentioned this pull request Sep 23, 2025
Closed
@janisz janisz mentioned this pull request Oct 3, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sourcery-ai sourcery-ai[bot] sourcery-ai[bot] left review comments

@rhybrillou rhybrillou rhybrillou approved these changes

@dashrews78 dashrews78 dashrews78 approved these changes

@mtodor mtodor Awaiting requested review from mtodor

Assignees

No one assigned

Labels

area/central area/postgres ci-performance-tests Runs the K6 based performance tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@janisz @rhacs-bot @rhybrillou @dashrews78