-
Notifications
You must be signed in to change notification settings - Fork 284
refactor(app/outbound): use svc::layers() to apply layers
#4191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
we apply a collection of `Layers` to an inner service in our metrics subsystem. doing so with `layer::mk()` and `Layer::layer()` is somewhat ungainly. this commit relies on `svc::layers()` to apply a sequence of telemetry layers in sequence. this should have no mechanical difference with the preëxisting code. Signed-off-by: katelyn martin <kate@buoyant.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a hard blocker, but two recommendations
| NewResponseDuration<T, ExtractRecordDurationParams<ResponseMetrics<T::StreamLabel>>, N>, | ||
| >, | ||
| >, | ||
| > + Clone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's generally good for our layers to be clone, it would preferable to preserve this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have not found a clear way to preserve this. i'm rather surprised by this, as i don't see any clear reason why this shouldn't be Clone. i tried wrapping the layer in a svc::layer::mk() call, with and without a call to into_inner() (below), but that didn't seem to work either!
i'd normally put this down, but the layers().push().push() syntax feels nice enough that i'm curious what's standing in the way here.
| svc::layers() | ||
| .push(duration) | ||
| .push(count_reqs) | ||
| .push(body_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can do an into_inner() to drop the Layers type wrapper...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i opened #4192, which introduces an into_inner() method.
we apply a collection of
Layersto an inner service in our metricssubsystem. doing so with
layer::mk()andLayer::layer()is somewhatungainly.
this commit relies on
svc::layers()to apply a sequence of telemetrylayers in sequence.
this should have no mechanical difference with the preëxisting code.
Signed-off-by: katelyn martin kate@buoyant.io