smithy-lang / smithy-kotlin

Smithy code generator for Kotlin (in development)

Apache License 2.0

72 stars 26 forks source link

feat: support for OkHttp thread metrics #1022

Open ianbotsf opened 6 months ago

ianbotsf commented 6 months ago

Issue \

894

Description of changes

This change adds metrics for the OkHttp engine's thread pool. It should be considered WIP until the following open questions are addressed:

How can we best test this in an automated fashion?
This change only adds support for OkHttp. CRT doesn't seem to have support for interrogating metrics about HTTP client thread counts. Is that good enough for now or should we push for enhanced support in CRT?
The additions to HttpClientMetrics eschew the existing pattern of pushing metrics to intermediate refs and the polling them from the async counter in favor of pulling the source data directly via a passed callback. This seems cleaner to me since it removes unnecessary state but I'm curious whether others think it's a good idea (and whether we'd potentially want to extend it to other async counters as appropriate).

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

lauzadis commented 6 months ago

I think unit tests for the metrics being emitted would be a good start. I'm not sure, is it easy to change the number of active threads in the OkHttp client, or is it more an internal implementation detail in the engine? If the latter, I think it would be sufficient to just check that the metrics are being emitted.
It would be nice to have both engines' metrics aligned but it can probably be solved on the longer term
I think it's a good idea

ianbotsf commented 6 months ago

I think unit tests for the metrics being emitted would be a good start. I'm not sure, is it easy to change the number of active threads in the OkHttp client, or is it more an internal implementation detail in the engine? If the latter, I think it would be sufficient to just check that the metrics are being emitted.

To be clear, you mean verify that a metric value exists but not what it is? Because this is multi-threaded behavior and we don't control the implementation of the underlying thread pool, it'll be difficult to achieve a stable expectation for number of total/active threads in a test case.

lauzadis commented 6 months ago

I think unit tests for the metrics being emitted would be a good start. I'm not sure, is it easy to change the number of active threads in the OkHttp client, or is it more an internal implementation detail in the engine? If the latter, I think it would be sufficient to just check that the metrics are being emitted.

To be clear, you mean verify that a metric value exists but not what it is? Because this is multi-threaded behavior and we don't control the implementation of the underlying thread pool, it'll be difficult to achieve a stable expectation for number of total/active threads in a test case.

Yes, just testing that a metric value exists if we don't have predictable behavior to test on.

sonarcloud[bot] commented 5 months ago

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud