Minimal initial benchmarks #631
Conversation
Added steps to run benchmark test on Github pull requests Added benchmark tests for Listing Apis/ApiVersions/ApiSpecs/Artifacts
Additionally run benchmark tests on PRs only when the registry server code has been updated.
It will be added as a separate PR
this will be addressed in a seperate PR.
Codecov Report
@@ Coverage Diff @@
## main #631 +/- ##
==========================================
- Coverage 51.97% 51.83% -0.15%
==========================================
Files 71 72 +1
Lines 6888 6911 +23
==========================================
+ Hits 3580 3582 +2
- Misses 2913 2934 +21
Partials 395 395
Continue to review full report at Codecov.
|
|
Should we consider integrating benchmarking tracking into our builds? For example: https://github.com/marketplace/actions/continuous-benchmarking-for-go |
@theganyo sgtm! if it isn't a gating concern for this PR, let's just discuss it in an issue. |
|
I attempted to run this locally and got errors... |
|
@theganyo it works for me... I was originally running on my Chromebook but just verified that it works on a MacBook (with a small change to line 21 of the script). How did you run this? |
|
|
|
Still getting the |
|
Well, I thought the problem was with wipeout but it seems instead that the function passed to Run() gets called multiple times without cleaning up after itself. Fixing... (I was getting errors for |
|
On another note re: schema. The wipeout code seems to make it clear we're not using foreign key constraints in the database? Manually maintaining DB integrity can be quite error prone... Shall I create an issue to address this? |
|
This seems to work now. We should probably add a wipeout_test.go, though. |
|
@theganyo Agreed - after starting a |
|
nit: Can we add a README.md for the pkg dir? |
|
Done (added pkg/README.md) |
| var _err error | ||
|
|
||
| // Wipeout deletes all resources in a project using the specified number of parallel worker jobs. | ||
| func Wipeout(ctx context.Context, client connection.Client, projectid string, jobs int) error { |
| ) | ||
|
|
||
| // Set this global variable in calling code to run verbosely. | ||
| var Verbose bool |
There was a problem hiding this comment.
Have we considered using flags or some other method for this config? I think this is the first use of config by setting a global var.
Also worth considering whether we need this at all, especially in the "minimal" initial benchmarks. We already have multiple logging levels that give control over the verbosity of output logs. If we want wipeout to be silent by default and optionally turned on, maybe we should just log at debug level instead of adding another dial for controlling log output.
There was a problem hiding this comment.
Thanks, I'll use a followup PR to change this to logging at Debug. How would we set that for a specific run of the test? (directly in code?)
| Artifact | ||
| ) | ||
|
|
||
| type deleteResourceTask struct { |
There was a problem hiding this comment.
These seems like a duplicate of deleteTask from the registry delete command. In #445 I suggested creating some reusable types for common tasks to avoid reinventing the wheel each time we want to do simple operations like creating/updating/deleting resources. Maybe now's the time?
The tasks can also be simpler by making them type-specific, which eliminates the "this should never happen" situation where we encounter an "unknown resource type" in these generic-esque tasks.
There was a problem hiding this comment.
partly addressed in #639
| } | ||
|
|
||
| func getApi(b *testing.B, ctx context.Context, client connection.Client, apiId string) error { | ||
| b.Helper() |
There was a problem hiding this comment.
Since this function doesn't actually fail/error/log using b, I don't think we gain anything from marking it as a helper (and can drop it entirely from the function).
| b.Logf("Unable to connect to registry server. Is it running?") | ||
| b.FailNow() |
| return fmt.Sprintf("projects/%s/locations/global", projectID) | ||
| } | ||
|
|
||
| func setup(b *testing.B) (context.Context, connection.Client) { |
There was a problem hiding this comment.
Why do we return a context here? This seems like something we should accept from callers.
There was a problem hiding this comment.
This reduced the boilerplate at the start of tests to a single call to setup(). Is there a style guide preference to not return a context from a function?
| return ctx, client | ||
| } | ||
|
|
||
| func teardown(b *testing.B, ctx context.Context, client connection.Client) { |
There was a problem hiding this comment.
We use ctx context.Context, t *testing.T elsewhere, so maybe we should swap b and ctx here. In other Google projects we prefer that ordering too.
This pares down @giteshk's benchmarks to a single set of CRUD tests on API resources that is hopefully easily extensible to other resources and scenarios. I'm not sure that we need to do this for every resource since the underlying operations don't vary much per resource. This alone seems enough to do some interesting comparisons of various hosting alternatives.