TRT-1989: add benchmarking db query tests#3533
Open
neisw wants to merge 2 commits into
Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@neisw: This pull request references TRT-1989 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
Contributor
WalkthroughThis PR extracts ProwJobRun query logic into a new exported JobDetailsReport helper, updates PrintJobDetailsReportFromDB to use it, and adds a Postgres benchmarking test suite exercising several DB queries and analysis cases. ChangesDB Query Refactoring and Performance Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/flags/postgres_benchmarking_test.go (2)
48-50: ⚡ Quick winRemove per-row logging from timed benchmark execution.
Printing each result row is included in measured duration and can overshadow the query signal.
Suggested fix
log.Printf("Found %d tests matching pattern for release %s", len(results), benchmarkRelease) - for _, r := range results { - log.Printf(" [%d] %s", r.ID, r.Name) - } return nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/flags/postgres_benchmarking_test.go` around lines 48 - 50, The test currently logs each row inside the timed benchmark loop (the for _, r := range results { log.Printf(" [%d] %s", r.ID, r.Name) }), which adds I/O to the measured duration; remove or move this per-row logging out of the timed section so the benchmark only measures query work (either delete the log.Printf call from the timed block or defer printing until after benchmarking using the same results slice).
83-85: ⚡ Quick winUse a fixed
reportEndfor stable benchmark baselines.Calling
time.Now()inside the case makes the queried window drift across runs, which hurts comparability.Suggested fix
-func getBenchmarkCases() []benchmarkCase { +func getBenchmarkCases(reportEnd time.Time) []benchmarkCase { @@ - jobRuns, err := api.JobDetailsReport(dbc, benchmarkRelease, + jobRuns, err := api.JobDetailsReport(dbc, benchmarkRelease, "periodic-ci-openshift-release-master-ci-4.22-e2e-aws-ovn", - time.Now()) + reportEnd)(Then pass a single captured value from each test, e.g.
reportEnd := time.Now().)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/flags/postgres_benchmarking_test.go` around lines 83 - 85, The test calls api.JobDetailsReport(..., time.Now()) directly which makes the report window vary across runs; capture a single fixed timestamp at the start of the test (e.g. reportEnd := time.Now()) and pass that variable into api.JobDetailsReport instead of calling time.Now() inline so the queried window is stable; update the call site where JobDetailsReport is invoked to use reportEnd and ensure any other places in the same test that need the same end time use the same captured variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/api/jobs.go`:
- Line 267: The two places that return raw errors (the statements returning
"nil, res.Error") should wrap the propagated error with context using fmt.Errorf
and %w; replace each raw return with something like fmt.Errorf("failed to
<describe operation> while calling res: %w", res.Error) so callers get
actionable context. Locate the returns that use the local variable res (the
"return nil, res.Error" occurrences) and wrap res.Error with fmt.Errorf
including a short description of the operation (e.g., "fetching job", "creating
job" or similar) and %w to preserve the original error.
---
Nitpick comments:
In `@pkg/flags/postgres_benchmarking_test.go`:
- Around line 48-50: The test currently logs each row inside the timed benchmark
loop (the for _, r := range results { log.Printf(" [%d] %s", r.ID, r.Name) }),
which adds I/O to the measured duration; remove or move this per-row logging out
of the timed section so the benchmark only measures query work (either delete
the log.Printf call from the timed block or defer printing until after
benchmarking using the same results slice).
- Around line 83-85: The test calls api.JobDetailsReport(..., time.Now())
directly which makes the report window vary across runs; capture a single fixed
timestamp at the start of the test (e.g. reportEnd := time.Now()) and pass that
variable into api.JobDetailsReport instead of calling time.Now() inline so the
queried window is stable; update the call site where JobDetailsReport is invoked
to use reportEnd and ensure any other places in the same test that need the same
end time use the same captured variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 13790764-57ce-47a6-bf6a-01ef331293f8
📒 Files selected for processing (2)
pkg/api/jobs.gopkg/flags/postgres_benchmarking_test.go
| if res.Error != nil { | ||
| log.Errorf("error querying %s ProwJobRuns from db: %v", jobSearchStr, res.Error) | ||
| return res.Error | ||
| return nil, res.Error |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify raw error returns in the modified file.
rg -n -C2 'return nil, res\.Error|return err' pkg/api/jobs.goRepository: openshift/sippy
Length of output: 396
🏁 Script executed:
# Check the file content around the error return lines and verify imports
head -n 30 pkg/api/jobs.go && echo -e "\n---MIDDLE SECTION---\n" && sed -n '260,285p' pkg/api/jobs.goRepository: openshift/sippy
Length of output: 1765
🏁 Script executed:
# Get the full function signatures to verify return types
sed -n '240,285p' pkg/api/jobs.goRepository: openshift/sippy
Length of output: 1650
Wrap propagated errors with context before returning.
Both error returns pass raw errors upstream. Add fmt.Errorf with context and %w wrapping to make failures diagnosable.
Suggested fix
if res.Error != nil {
- return nil, res.Error
+ return nil, fmt.Errorf("querying prow job runs for release %q and job pattern %q: %w", release, jobSearchStr, res.Error)
}
@@
if err != nil {
log.Errorf("error querying %s ProwJobRuns from db: %v", jobSearchStr, err)
- return err
+ return fmt.Errorf("building job details report for %q: %w", jobSearchStr, err)
}Applies to lines 267 and 280 per coding guidelines: "Errors should be wrapped with context using fmt.Errorf with %w."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/api/jobs.go` at line 267, The two places that return raw errors (the
statements returning "nil, res.Error") should wrap the propagated error with
context using fmt.Errorf and %w; replace each raw return with something like
fmt.Errorf("failed to <describe operation> while calling res: %w", res.Error) so
callers get actionable context. Locate the returns that use the local variable
res (the "return nil, res.Error" occurrences) and wrap res.Error with fmt.Errorf
including a short description of the operation (e.g., "fetching job", "creating
job" or similar) and %w to preserve the original error.
Contributor
|
Scheduling required tests: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/flags/postgres_benchmarking_test.go (1)
59-168: ⚡ Quick winStabilize benchmark time windows to keep timings comparable.
Each case evaluates
time.Now()at call time, so the queried window shifts between cases/iterations. That adds data drift/noise to results. Capture one reference timestamp per test run and pass it into cases.Proposed refactor
type benchmarkCase struct { name string - fn func(dbc *db.DB) error + fn func(dbc *db.DB, now time.Time) error } -func getBenchmarkCases() []benchmarkCase { +func getBenchmarkCases(now time.Time) []benchmarkCase { return []benchmarkCase{ { name: "JobDetails", - fn: func(dbc *db.DB) error { + fn: func(dbc *db.DB, now time.Time) error { jobRuns, err := api.JobDetailsReport(dbc, benchmarkRelease, - benchmarkJobName, time.Now()) + benchmarkJobName, now) ... }, }, { name: "TestAnalysisOverall", - fn: func(dbc *db.DB) error { + fn: func(dbc *db.DB, now time.Time) error { results, err := api.GetTestAnalysisOverallFromDB(dbc, nil, - benchmarkRelease, benchmarkTestName, time.Now()) + benchmarkRelease, benchmarkTestName, now) ... }, }, { name: "TestAnalysisPassRate", - fn: func(dbc *db.DB) error { + fn: func(dbc *db.DB, now time.Time) error { ... res := dbc.DB.Raw(query.QueryTestAnalysis, - time.Now().Add(-24*14*time.Hour), + now.Add(-24*14*time.Hour), benchmarkTestName, []string{benchmarkJobName}).Scan(&result) ... }, }, } } func Test_BenchmarkIndividual(t *testing.T) { dbc := getBenchmarkDBClient(t) + now := time.Now() iterations := 3 - cases := getBenchmarkCases() + cases := getBenchmarkCases(now) ... - err := bc.fn(dbc) + err := bc.fn(dbc, now) ... } func Test_BenchmarkGroup(t *testing.T) { dbc := getBenchmarkDBClient(t) + now := time.Now() iterations := 1 - cases := getBenchmarkCases() + cases := getBenchmarkCases(now) ... - err := bc.fn(dbc) + err := bc.fn(dbc, now) ... }Also applies to: 189-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/flags/postgres_benchmarking_test.go` around lines 59 - 168, The benchmark cases call time.Now() inside each case, causing shifting windows and noisy results; capture a single reference timestamp per test run and use it in all cases. Modify getBenchmarkCases to accept a reference time (e.g., func getBenchmarkCases(ref time.Time) []benchmarkCase) or capture a refTime variable from the caller and close over it; replace all time.Now() uses inside the cases (JobDetails -> api.JobDetailsReport, GetTestAnalysisOverallFromDB, GetTestAnalysisByJobFromDB calls, and the TestAnalysisPassRate raw query) with the provided ref time (and adjust the PassRate window using ref.Add(...)). Update the test harness that constructs/iterates these benchmark cases to pass the same reference timestamp into getBenchmarkCases (or create refTime before calling getBenchmarkCases if you chose closure).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/flags/postgres_benchmarking_test.go`:
- Around line 170-187: The helper getBenchmarkDBClient currently returns a
*db.DB without closing it; after successfully creating dbc in
getBenchmarkDBClient register a t.Cleanup that closes the client (e.g.
t.Cleanup(func(){ _ = dbc.Close() } ) or log any close error) so the connection
is closed when the test finishes; place the t.Cleanup call immediately after the
dbc, err check and before returning dbc.
---
Nitpick comments:
In `@pkg/flags/postgres_benchmarking_test.go`:
- Around line 59-168: The benchmark cases call time.Now() inside each case,
causing shifting windows and noisy results; capture a single reference timestamp
per test run and use it in all cases. Modify getBenchmarkCases to accept a
reference time (e.g., func getBenchmarkCases(ref time.Time) []benchmarkCase) or
capture a refTime variable from the caller and close over it; replace all
time.Now() uses inside the cases (JobDetails -> api.JobDetailsReport,
GetTestAnalysisOverallFromDB, GetTestAnalysisByJobFromDB calls, and the
TestAnalysisPassRate raw query) with the provided ref time (and adjust the
PassRate window using ref.Add(...)). Update the test harness that
constructs/iterates these benchmark cases to pass the same reference timestamp
into getBenchmarkCases (or create refTime before calling getBenchmarkCases if
you chose closure).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 386c330a-b97f-4948-90e3-1eb11c7841d2
📒 Files selected for processing (1)
pkg/flags/postgres_benchmarking_test.go
Comment on lines
+170
to
+187
| func getBenchmarkDBClient(t *testing.T) *db.DB { | ||
| t.Helper() | ||
| dsn := os.Getenv("db_benchmarking_dsn") | ||
| if dsn == "" { | ||
| t.Skip("skipping: set db_benchmarking_dsn to run") | ||
| } | ||
|
|
||
| dbFlags := &PostgresFlags{ | ||
| LogLevel: 4, | ||
| DSN: dsn, | ||
| } | ||
|
|
||
| dbc, err := dbFlags.GetDBClient() | ||
| if err != nil { | ||
| t.Fatalf("couldn't get DB client: %v", err) | ||
| } | ||
| return dbc | ||
| } |
Contributor
There was a problem hiding this comment.
Close the benchmark DB client with t.Cleanup to avoid leaked connections.
The helper opens a DB client but never closes it. Register cleanup when the client is created.
Proposed fix
func getBenchmarkDBClient(t *testing.T) *db.DB {
t.Helper()
dsn := os.Getenv("db_benchmarking_dsn")
if dsn == "" {
t.Skip("skipping: set db_benchmarking_dsn to run")
}
@@
dbc, err := dbFlags.GetDBClient()
if err != nil {
t.Fatalf("couldn't get DB client: %v", err)
}
+ sqlDB, err := dbc.DB.DB()
+ if err != nil {
+ t.Fatalf("couldn't get sql DB handle: %v", err)
+ }
+ t.Cleanup(func() {
+ if err := sqlDB.Close(); err != nil {
+ t.Logf("failed to close DB client: %v", err)
+ }
+ })
return dbc
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/flags/postgres_benchmarking_test.go` around lines 170 - 187, The helper
getBenchmarkDBClient currently returns a *db.DB without closing it; after
successfully creating dbc in getBenchmarkDBClient register a t.Cleanup that
closes the client (e.g. t.Cleanup(func(){ _ = dbc.Close() } ) or log any close
error) so the connection is closed when the test finishes; place the t.Cleanup
call immediately after the dbc, err check and before returning dbc.
Contributor
|
Scheduling required tests: |
Contributor
|
@neisw: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Looking to establish a baseline set of tests we can use to measure improvement
Summary by CodeRabbit