enhancement(app): allow configuring static handles on DynamicAPIBuilder#1632
Conversation
dd-octo-sts
Bot
added
the
area/observability
tobz
changed the title
DynamicAPIBuilder
Binary Size Analysis (Agent Data Plane)Target: 1f5e022 (baseline) vs 6f8b5e2 (comparison) diff
|
| Module | File Size | Symbols |
|---|---|---|
core |
-29.35 KiB | 850 |
rustls |
-11.31 KiB | 4 |
hyper_util |
+10.02 KiB | 7 |
hyper |
-9.19 KiB | 63 |
std |
-7.37 KiB | 29 |
serde_json |
+5.66 KiB | 11 |
saluki_app::logging::build_output_stack |
+5.55 KiB | 1 |
serde_core |
+5.21 KiB | 34 |
tokio_rustls |
+4.95 KiB | 8 |
http |
+4.85 KiB | 29 |
serde_path_to_error |
-4.37 KiB | 10 |
h2 |
-4.16 KiB | 86 |
alloc |
+3.88 KiB | 36 |
saluki_app::logging::layer |
+3.75 KiB | 5 |
tonic |
+3.35 KiB | 7 |
anon.ce161719d13b82de2ef20076d926a51e.267.llvm.13781261162817590056 |
-3.24 KiB | 1 |
anon.7f89611b641a2d0028b07647399f351e.747.llvm.10929549773975020062 |
+3.24 KiB | 1 |
figment |
-3.20 KiB | 15 |
[sections] |
-2.75 KiB | 7 |
tokio |
+2.72 KiB | 87 |
Detailed Symbol Changes
FILE SIZE VM SIZE
-------------- --------------
[NEW] +16.7Ki [NEW] +16.5Ki _<saluki_app::dynamic_api::DynamicAPIBuilder as saluki_core::runtime::supervisor::Supervisable>::initialize::_{{closure}}::h6285bfc5ab742e9f
+237% +13.5Ki +242% +13.5Ki h2::proto::connection::DynConnection<B>::recv_frame::hfab6ab4a3734ad96
[NEW] +8.04Ki [NEW] +7.81Ki _<saluki_app::dynamic_api::DynamicAPIBuilder as saluki_core::runtime::supervisor::Supervisable>::initialize::_{{closure}}::_{{closure}}::h1c0eefe8e8dbaddb
+586% +7.88Ki +631% +7.88Ki tokio_rustls::common::Stream<IO,C>::read_io::he30dc07a3aea22c7
+64% +5.55Ki +64% +5.55Ki saluki_app::logging::build_output_stack::hcd3ed91e2e899dd8
[NEW] +4.90Ki [NEW] +4.71Ki serde_json::value::de::_<impl serde_core::de::Deserialize for serde_json::value::Value>::deserialize::he3651b66be1638db
+167% +4.30Ki +179% +4.30Ki _<hyper_util::server::conn::auto::Connection<I,S,E> as core::future::future::Future>::poll::h771733023775cf41
+145% +3.71Ki +155% +3.71Ki _<hyper_util::server::conn::auto::Connection<I,S,E> as core::future::future::Future>::poll::heef3788e09e28a94
[NEW] +3.24Ki [NEW] +74 anon.7f89611b641a2d0028b07647399f351e.747.llvm.10929549773975020062
[DEL] -2.83Ki [DEL] -2.73Ki crossbeam_channel::flavors::list::Channel<T>::recv::h43ef797a5cfb3e45
[DEL] -2.91Ki [DEL] -2.80Ki h2::proto::streams::streams::DynStreams<B>::recv_headers::h13168d11d4402295
[DEL] -2.95Ki [DEL] -2.84Ki h2::proto::streams::streams::DynStreams<B>::recv_push_promise::h5a479ee6beaf6c45
[DEL] -3.24Ki [DEL] -74 anon.ce161719d13b82de2ef20076d926a51e.267.llvm.13781261162817590056
-29.5% -3.77Ki -29.9% -3.77Ki _<rustls::error::Error as core::clone::Clone>::clone::h5d268dcb651f6988
[DEL] -3.96Ki [DEL] -3.80Ki _<hyper::proto::h2::server::Server<T,S,B,E> as core::future::future::Future>::poll::h33e066a292d6fc33
[DEL] -4.10Ki [DEL] -3.95Ki _<hyper::proto::h2::server::Server<T,S,B,E> as core::future::future::Future>::poll::h7c94ce4cd06944c9
[DEL] -4.78Ki [DEL] -4.62Ki _<figment::value::de::ConfiguredValueDe<I> as serde_core::de::Deserializer>::deserialize_any::h8ef7a02b6f517dd4
[DEL] -7.57Ki [DEL] -7.46Ki rustls::conn::ConnectionCore<Data>::process_new_packets::h18cb26a9c4598de9
[DEL] -9.56Ki [DEL] -9.33Ki _<saluki_app::dynamic_api::DynamicAPIBuilder as saluki_core::runtime::supervisor::Supervisable>::initialize::_{{closure}}::_{{closure}}::h323904c022f4abf2
[DEL] -16.0Ki [DEL] -15.8Ki _<saluki_app::dynamic_api::DynamicAPIBuilder as saluki_core::runtime::supervisor::Supervisable>::initialize::_{{closure}}::he10070fd5963cb28
-0.3% -14.9Ki -0.5% -17.5Ki [3448 Others]
-0.0% -8.74Ki -0.0% -10.6Ki TOTAL
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1a100b459
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let resetable = router.clone().reset_fallback(); | ||
| match try_merge_router(&merged, id, &resetable) { |
There was a problem hiding this comment.
When a dynamic HTTP handler registers a valid Router::fallback/fallback_service catch-all, this shared rebuild path now strips it before merging because reset_fallback() is applied to every handler, not just gRPC handlers. The previous code merged HTTP dynamic routers as-is, so those catch-all HTTP routes would be served; after this change they silently become the default 404, while the comment only calls out Tonic's gRPC fallback collision. Consider applying fallback reset only on the gRPC rebuild path or otherwise preserving HTTP fallbacks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is true, but also, we really don't want random route layers defining a fallback for the whole merged router... that wouldn't make sense.
Regression Detector (Agent Data Plane)Regression Detector ResultsRun ID: 23b859a2-b31f-4d68-903e-27cea3526774 Baseline: 4aeb9c7 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | otlp_ingest_logs_5mb_cpu | % cpu utilization | +1.68 | [-2.84, +6.21] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_throughput | ingress throughput | -0.02 | [-0.14, +0.11] | 1 | (metrics) (profiles) (logs) |
| ✅ | otlp_ingest_logs_5mb_memory | memory utilization | -9.84 | [-10.20, -9.49] | 1 | (metrics) (profiles) (logs) |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | dsd_uds_512kb_3k_contexts_cpu | % cpu utilization | +11.02 | [-50.47, +72.52] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_10mb_3k_contexts_cpu | % cpu utilization | +3.22 | [-28.70, +35.14] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_cpu | % cpu utilization | +1.68 | [-2.84, +6.21] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_metrics_5mb_memory | memory utilization | +1.67 | [+1.48, +1.86] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_5mb_cpu | % cpu utilization | +1.24 | [-0.98, +3.45] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_100mb_3k_contexts_cpu | % cpu utilization | +0.41 | [-5.31, +6.14] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_512kb_3k_contexts_memory | memory utilization | +0.28 | [+0.14, +0.42] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_transform_5mb_throughput | ingress throughput | +0.16 | [+0.09, +0.23] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_heavy | memory utilization | +0.15 | [+0.02, +0.28] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_filtering_5mb_memory | memory utilization | +0.09 | [-0.16, +0.34] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_filtering_5mb_throughput | ingress throughput | +0.07 | [+0.00, +0.15] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_10mb_3k_contexts_memory | memory utilization | +0.07 | [-0.08, +0.23] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_5mb_throughput | ingress throughput | +0.06 | [-0.01, +0.13] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_100mb_3k_contexts_memory | memory utilization | +0.03 | [-0.12, +0.19] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_metrics_5mb_throughput | ingress throughput | +0.02 | [-0.15, +0.19] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_100mb_3k_contexts_throughput | ingress throughput | +0.01 | [-0.02, +0.05] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_1mb_3k_contexts_throughput | ingress throughput | +0.00 | [-0.05, +0.06] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_10mb_3k_contexts_throughput | ingress throughput | -0.00 | [-0.18, +0.18] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_512kb_3k_contexts_throughput | ingress throughput | -0.01 | [-0.06, +0.05] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_throughput | ingress throughput | -0.02 | [-0.14, +0.11] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_5mb_memory | memory utilization | -0.03 | [-0.19, +0.14] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_ultraheavy | memory utilization | -0.06 | [-0.19, +0.07] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_500mb_3k_contexts_memory | memory utilization | -0.09 | [-0.24, +0.05] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_500mb_3k_contexts_cpu | % cpu utilization | -0.11 | [-1.60, +1.38] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_low | memory utilization | -0.12 | [-0.29, +0.04] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_idle | memory utilization | -0.16 | [-0.20, -0.12] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_transform_5mb_memory | memory utilization | -0.34 | [-0.51, -0.18] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_medium | memory utilization | -0.36 | [-0.53, -0.19] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_1mb_3k_contexts_memory | memory utilization | -0.41 | [-0.56, -0.26] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_metrics_5mb_cpu | % cpu utilization | -1.46 | [-7.62, +4.70] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_500mb_3k_contexts_throughput | ingress throughput | -1.81 | [-1.96, -1.66] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_filtering_5mb_cpu | % cpu utilization | -1.84 | [-4.08, +0.40] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_transform_5mb_cpu | % cpu utilization | -2.00 | [-4.15, +0.15] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_1mb_3k_contexts_cpu | % cpu utilization | -4.36 | [-59.34, +50.61] | 1 | (metrics) (profiles) (logs) |
| ✅ | otlp_ingest_logs_5mb_memory | memory utilization | -9.84 | [-10.20, -9.49] | 1 | (metrics) (profiles) (logs) |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gates_rss_dsd_heavy | memory_usage | 10/10 | 122.66MiB ≤ 140MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_dsd_low | memory_usage | 10/10 | 39.50MiB ≤ 50MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_dsd_medium | memory_usage | 10/10 | 60.46MiB ≤ 75MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_dsd_ultraheavy | memory_usage | 10/10 | 174.00MiB ≤ 200MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_idle | memory_usage | 10/10 | 26.96MiB ≤ 40MiB | (metrics) (profiles) (logs) |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
tobz
force-pushed
the
tobz/dynamic-api-partial-privileged
branch
from
d1a100b to
6f8b5e2
Compare
2 participants
Summary
This PR adds the ability to add static HTTP/gRPC routes ("handlers") to
DynamicAPIBuilderin order to facilitate gradual migration fromAPIBuilder.Prior to this PR,
DynamicAPIBuilderwas only usable if all desired API routes were published dynamically. While this is currently the case for the routes on the unprivileged API endpoint, most (all?) of the routes on the privileged API endpoint are "static": they don't come from supervised workers or haven't yet been migrated to do so.In order to let us get the plumbing in place to automatically utilize dynamically published routes destined for the privileged API endpoint... we can now manually add static handlers to
DynamicAPIBuilder. We made a few changes to support this:with_handler,with_optional_handler, andwith_grpc_servicetoDynamicAPIBuilder, mimicking the methods ofAPIBuilderexactlyDynamicAPIBuilderto handle using the static handlers as the "base", taking priority over dynamically published routesaxumis a little specific in this regard)Switching over to using
DynamicAPIBuilderfor the privileged API endpoint will be left as a follow-up PR.Change Type
How did you test this PR?
References
DADP-2