misc: add service-level benchmarks #1006

ianbotsf · 2023-07-28T21:45:17Z

Issue #

Closes #968

Description of changes

Add a new module :tests:benchmarks:service-benchmarks to run service-level benchmarks. See that module's README.md for more details.

There's a lot in the PR but here are some highlights:

ServiceBenchmark.kt defines the benchmark structure and BenchmarkHarness.kt's main method is the application entry point. It may be easiest to start your review from those two places.
I've added a way to bootstrap service dependencies for a given module (i.e., ./gradlew :tests:benchmarks:service-benchmarks:bootstrapAll). Also add a way to gracefully skip building a project when its service dependencies haven't been built. I think this could be a template for how we build higher-level libraries (e.g., S3 Transfer Manager) in the same repo as the SDK itself. If we like it, I think the build logic could be abstracted to be more reusable in future modules.
BenchmarkTelemetryProvider defines the metrics to capture, currently just SDK overhead. We should think about whether there are other metrics we'd want to capture as a baseline or in eventual dashboards.
MetricAggregator defines the statistics to compute on captured metrics, currently count, minimum, average, median/p50, p90, p99, and maximum. At the number of iterations we run, I think p90 is the most interesting.
Our overhead seems consistently highest on Cloudwatch and consistently lowest on DynamoDB. I haven't dug too deeply into why yet.

Companion PR: smithy-lang/smithy-kotlin#908

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

lauzadis

I like the logic around bootstrapAll and agree it'd be great to use for things like S3 Transfer Manager!

lauzadis · 2023-08-07T14:13:56Z

tests/benchmarks/service-benchmarks/README.md

+|                       | Overhead (ms) |    n |   min |   avg |   med |   p90 |    p99 |    max |
+| :---                  |          ---: | ---: |  ---: |  ---: |  ---: |  ---: |   ---: |   ---: |
+| **S3**                |               |      |       |       |       |       |        |        |
+|   —HeadObject         |               | 1618 | 0.340 | 0.605 | 0.417 | 0.638 |  4.864 | 14.672 |


nit: em dash not required, it's easy to read the table without them

lauzadis · 2023-08-07T14:14:59Z

tests/benchmarks/service-benchmarks/build.gradle.kts

+}
+
+val requiredServices = setOf(
+    // Top 7 services called by Kotlin SDK customers as of 7/25/2023


nit: would be helpful to note this is in descending order

lauzadis · 2023-08-07T14:15:13Z

tests/benchmarks/service-benchmarks/build.gradle.kts

+    "pinpoint",
+
+    // Services required as prerequisites for setup
+    "iam", // Create roles for SNS::AssumeRole


nit: STS::AssumeRole

aajtodd

Couple minor comments/suggestions/cleanup but nothing major. Overall this looks great!

aajtodd · 2023-08-08T11:43:39Z

...s/benchmarks/service-benchmarks/common/src/aws/sdk/kotlin/benchmarks/service/ResultsTable.kt

+private const val NAME_FIELD = "name"
+private const val COUNT_FIELD = "n"
+
+private typealias Results = Map<String, Map<String, Map<String, MetricSummary>>>


nit: Might help to explain the format of this what is this a key-value mapping of?

aajtodd · 2023-08-08T11:46:48Z

...chmarks/common/src/aws/sdk/kotlin/benchmarks/service/telemetry/BenchmarkTelemetryProvider.kt

+            override fun createUpDownCounter(name: String, units: String?, description: String?) =
+                NoOpUpDownCounter
+
+            override fun createAsyncUpDownCounter(


comment: In future I could see filling in the rest of these to be able to benchmark connection usage, request concurrency, etc. I don't think we need to do anything right now though

Yeah, I actually tested out a few more metrics while working on this but decided just "overhead" was the clearest and simplest place to start. There's room to grow for sure.

aajtodd · 2023-08-08T11:47:58Z

...chmarks/common/src/aws/sdk/kotlin/benchmarks/service/telemetry/BenchmarkTelemetryProvider.kt

+)
+
+@ExperimentalApi
+class BenchmarkTelemetryProvider(private val metricAggregator: MetricAggregator) : TelemetryProvider {


comment: I'm thinking we need a TelemetryProviderBuilder that allows you to easily override just the providers you want 🤔 . Not needed in this PR though of course.

aajtodd · 2023-08-08T11:49:13Z

tests/benchmarks/service-benchmarks/README.md

+* `./gradlew build`
+  This builds the whole SDK.
+* `./gradlew :tests:benchmarks:service-benchmarks:run`
+  This runs the benchmark suite and prints the results to the console formatted as a Markdown table.


comment: Markdown is nice but I'm wondering if we want to also support JSON at some point. It will be easier to do delta comparisons. IIRC java v2 stores their baseline in a json file in the repo.

Yes, we absolutely want to support other formats at some point. I can imagine an additional parameter to the benchmark target that sets the output type: markdown, JSON, maybe even directly to CloudWatch via environment credentials.

aajtodd · 2023-08-08T11:54:09Z

tests/benchmarks/service-benchmarks/README.md

+|----------------|------------------|-----------|
+| EC2 m5.4xlarge | Amazon Linux 2   | 7/28/2023 |
+
+### Results


comment: Eventually probably want to benchmark by HTTP client as well.

aajtodd · 2023-08-08T11:55:27Z

...vice-benchmarks/common/src/aws/sdk/kotlin/benchmarks/service/definitions/ServiceBenchmark.kt

+import aws.smithy.kotlin.runtime.client.SdkClient
+import kotlin.time.Duration
+
+interface ServiceBenchmark<C : SdkClient> {


fix: Docs on all of these types/methods would be helpful

aajtodd · 2023-08-08T11:56:56Z

...s/service-benchmarks/common/src/aws/sdk/kotlin/benchmarks/service/definitions/S3Benchmark.kt

+        override suspend fun setup(client: S3Client) {
+            client.putObject {
+                bucket = bucketName
+                key = KEY


Question: Any concern about key/prefix throttling? I know in our canary we use a wide range of keys to avoid throttling.

No concern. Because we're running single-threaded benchmarks, our transaction rate is solely dependent on the end-to-end operation time. Given that each roundtrip takes at least 1ms, we're nowhere close to the throttling limit of 3500 TPS per prefix.

aajtodd · 2023-08-08T11:58:06Z

.../service-benchmarks/common/src/aws/sdk/kotlin/benchmarks/service/definitions/StsBenchmark.kt

+                iamRoleArn = resp.role!!.arn!!
+            }
+
+            // It takes a while for newly-created roles to fully propagate to STS. In the meantime, trying to assume the


comment: Seems like a waitUntilRoleExists waiter should exit 🤔

waitUntilRoleExists does exist but it doesn't do what we want. The role is created and usable in IAM almost immediately but that's only the first part. The real long wait is the propagation to STS, which appears to be slow and may be using multi-node replication since it can be transiently available for a short time. 🤦‍♂️

aajtodd · 2023-08-08T12:01:09Z

tests/benchmarks/service-benchmarks/build.gradle.kts

+tasks.register("bootstrapAll") {
+    val bootstrapArg = requiredServices.joinToString(",") { "+$it" }
+    val bootstrapProj = project(":codegen:sdk")
+    bootstrapProj.ext.set("aws.services", bootstrapArg)


comment: This is ok for now but this inter-project task dependency is probably not great. This is fine for now though as I have no better suggestion at the moment.

What are the shortcomings you see? What kinds of problems may we encounter if we continue using this pattern here and elsewhere?

sonarcloud · 2023-08-08T22:13:19Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

No Coverage information
0.0% Duplication

misc: add service-level benchmarks

a7f084d

ianbotsf requested a review from a team as a code owner July 28, 2023 21:45

ianbotsf mentioned this pull request Jul 28, 2023

misc: tweak metrics to support service-level benchmarks smithy-lang/smithy-kotlin#908

Merged

lauzadis approved these changes Aug 7, 2023

View reviewed changes

aajtodd reviewed Aug 8, 2023

View reviewed changes

ianbotsf added 4 commits August 8, 2023 20:59

addressing PR feedback

0c80ffc

lint

987fb96

Merge remote-tracking branch 'origin/main' into service-benchmarks

7134a58

update benchmark results

7edb8b0

aajtodd approved these changes Aug 9, 2023

View reviewed changes

ianbotsf merged commit 3a20e42 into main Aug 9, 2023
11 of 13 checks passed

ianbotsf deleted the service-benchmarks branch August 9, 2023 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: add service-level benchmarks #1006

misc: add service-level benchmarks #1006

ianbotsf commented Jul 28, 2023

lauzadis left a comment

lauzadis Aug 7, 2023

lauzadis Aug 7, 2023

lauzadis Aug 7, 2023

aajtodd left a comment

aajtodd Aug 8, 2023

aajtodd Aug 8, 2023

ianbotsf Aug 8, 2023

aajtodd Aug 8, 2023

aajtodd Aug 8, 2023

ianbotsf Aug 8, 2023

aajtodd Aug 8, 2023

aajtodd Aug 8, 2023

aajtodd Aug 8, 2023

ianbotsf Aug 8, 2023

aajtodd Aug 8, 2023

ianbotsf Aug 8, 2023

aajtodd Aug 8, 2023

ianbotsf Aug 8, 2023

sonarcloud bot commented Aug 8, 2023

misc: add service-level benchmarks #1006

misc: add service-level benchmarks #1006

Conversation

ianbotsf commented Jul 28, 2023

Issue #

Description of changes

lauzadis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aajtodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 8, 2023