Skip to content

Commit ef90dd5

Browse files
Brian Eckermeta-codesync[bot]
authored andcommitted
Add a pure-virtual getFilterName() to HTTPSourceFilter
Summary: NOTE: this is one of two candidate designs, pushed as a draft so it can be compared against D114801323 (same goal, non-pure virtual with a default). Not final. `HTTPMessageFilter` has had `getFilterName()` for a long time, and revproxy uses it to record which filters a response passed through (the `var.resp_filter_chain` variable, which ends up in CDN access logs and in TRC). The coro filter hierarchy has no equivalent, so a coro filter chain cannot be described at all. This adds the same accessor to `HTTPSourceFilter` and implements it on every subclass. It is deliberately pure, so a filter cannot land without a name. That is a bigger change than it sounds: `HTTPSource` is abstract, but `HTTPSourceFilter` is not -- it implements all three of `HTTPSource`'s pure virtuals by delegating to `source_`, which is exactly why subclasses can override only what they want to intercept. A pure virtual makes `HTTPSourceFilter` abstract for the first time, so every leaf subclass in the repo has to implement the accessor, including test fixtures. That is ~55 classes across proxygen, bigcache, manifold, c2p, crackerjack, edgetee, tgc, otel_gateway, observability query gateway, sandlet and oil. One site (`CacheFillRequestTest`) constructed `HTTPSourceFilter` directly as a pass-through and now needs a named subclass to compile at all. Filters that have a non-coro counterpart return the same name -- `StripPerHop`, `Decompression`, `ResponseTracing`, `RequestTracing`, `websocket-proxy` -- so a chain recorded from either handler is comparable. The rest are named after themselves, and test fixtures take a `Test` prefix. Two notes: the per-hop stripping filter in `HTTPCoroRevProxyHandler` was an anonymous `TransformFilter` configured with lambdas and had no type to hang a name on, so it becomes a named `StripPerHopSourceFilter`; and `BodyDecompressionFilter` deliberately has no override, since inheriting `Decompression` from `DecompressionIngressFilter` is the correct answer. No behavior change -- nothing reads these names until D114778859 on top. The known cost of the pure form is that generic filters can only be named by type, so `Transform`, `Visitor` and `CompletionTransform` are uninformative in a logged chain even though those instances are doing quite specific jobs. Reviewed By: hanidamlaj Differential Revision: D114801329 fbshipit-source-id: 1a9b5be2202e2b623f13d111f07a838adb3d2283
1 parent 5ffc71d commit ef90dd5

17 files changed

Lines changed: 76 additions & 0 deletions

proxygen/lib/http/coro/HTTPHybridSource.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@ namespace proxygen::coro {
1919
*/
2020
class HTTPHybridSource : public HTTPSourceFilter {
2121
public:
22+
[[nodiscard]] std::string_view getFilterName() const noexcept override {
23+
return "HybridSource";
24+
}
2225
HTTPHybridSource(std::unique_ptr<HTTPMessage> headers, HTTPSource* source)
2326
: headerEvent_(std::move(headers), !source) {
2427
if (source) {

proxygen/lib/http/coro/HTTPSourceFilter.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
#include "proxygen/lib/http/coro/HTTPSource.h"
1212
#include <folly/logging/xlog.h>
13+
#include <string_view>
1314

1415
namespace proxygen::coro {
1516

@@ -49,6 +50,8 @@ class HTTPSourceFilter : public HTTPSource {
4950
}
5051
}
5152

53+
[[nodiscard]] virtual std::string_view getFilterName() const noexcept = 0;
54+
5255
// HTTPSource overrides
5356
folly::coro::Task<HTTPHeaderEvent> readHeaderEvent() override {
5457
return readHeaderEventImpl(/*deleteOnDone=*/true);

proxygen/lib/http/coro/HTTPSourceHolder.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ namespace proxygen::coro {
1616
// read, or stopReading is called on the source.
1717
class HTTPSourceHolder : public HTTPSourceFilter {
1818
public:
19+
[[nodiscard]] std::string_view getFilterName() const noexcept override {
20+
return "SourceHolder";
21+
}
1922
HTTPSourceHolder() = default;
2023

2124
/* implicit */ HTTPSourceHolder(HTTPSource* source)

proxygen/lib/http/coro/filters/CompressionFilter.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ using FilterParams = proxygen::CompressionFilterUtils::FilterParams;
1818
namespace proxygen::coro {
1919
class CompressionFilter : public HTTPSourceFilter {
2020
public:
21+
[[nodiscard]] std::string_view getFilterName() const noexcept override {
22+
return "Compression";
23+
}
2124
CompressionFilter(HTTPSource* source,
2225
std::shared_ptr<folly::Optional<FilterParams>> params)
2326
: HTTPSourceFilter(source), params_(std::move(params)) {

proxygen/lib/http/coro/filters/DecompressionFilter.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ namespace proxygen::coro {
2222
*/
2323
class DecompressionEgressFilter : public MutateFilter {
2424
public:
25+
[[nodiscard]] std::string_view getFilterName() const noexcept override {
26+
return "DecompressionEgress";
27+
}
2528
DecompressionEgressFilter(HTTPSource* source = nullptr);
2629
};
2730

@@ -50,6 +53,11 @@ class DecompressionIngressFilter : public HTTPSourceFilter {
5053
folly::coro::Task<HTTPBodyEvent> readBodyEvent(
5154
uint32_t max = std::numeric_limits<uint32_t>::max()) override;
5255

56+
// Matches the non-coro DecompressionFilter's name.
57+
[[nodiscard]] std::string_view getFilterName() const noexcept override {
58+
return "Decompression";
59+
}
60+
5361
static bool compressionSupported(HTTPMessage& msg);
5462

5563
protected:

proxygen/lib/http/coro/filters/HTTPRedirectHandler.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,9 @@ class HTTPRedirectHandler {
6363
private:
6464
class RequestFilter : public HTTPSourceFilter {
6565
public:
66+
[[nodiscard]] std::string_view getFilterName() const noexcept override {
67+
return "RedirectRequest";
68+
}
6669
explicit RequestFilter(HTTPRedirectHandler& handler) : handler_(handler) {
6770
}
6871

@@ -86,6 +89,9 @@ class HTTPRedirectHandler {
8689

8790
class ResponseFilter : public HTTPSourceFilter {
8891
public:
92+
[[nodiscard]] std::string_view getFilterName() const noexcept override {
93+
return "RedirectResponse";
94+
}
8995
explicit ResponseFilter(HTTPRedirectHandler& handler) : handler_(handler) {
9096
}
9197

proxygen/lib/http/coro/filters/Logger.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,9 @@ class Logger {
158158

159159
class Filter : public HTTPSourceFilter {
160160
public:
161+
[[nodiscard]] std::string_view getFilterName() const noexcept override {
162+
return "Logger";
163+
}
161164
enum class Direction { REQUEST, RESPONSE };
162165
explicit Filter(Direction dir) : direction_(dir) {
163166
done = folly::coro::makePromiseContract<void>();

proxygen/lib/http/coro/filters/MutateFilter.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@ namespace proxygen::coro {
2121
*/
2222
class MutateFilter : public HTTPSourceFilter {
2323
public:
24+
[[nodiscard]] std::string_view getFilterName() const noexcept override {
25+
return "Mutate";
26+
}
2427
using HeaderMutateFn = std::function<void(HTTPHeaderEvent&)>;
2528
using BodyMutateFn = std::function<void(HTTPBodyEvent&)>;
2629

proxygen/lib/http/coro/filters/RateLimitFilter.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@ namespace proxygen::coro {
2020
*/
2121
class RateLimitFilter : public HTTPSourceFilter {
2222
public:
23+
[[nodiscard]] std::string_view getFilterName() const noexcept override {
24+
return "RateLimit";
25+
}
2326
explicit RateLimitFilter(HTTPSource* source);
2427
explicit RateLimitFilter(HTTPSource* source,
2528
std::chrono::seconds maxDelay,

proxygen/lib/http/coro/filters/RequestContextFilterFactory.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@ namespace proxygen::coro {
1414
namespace {
1515
class RequestContextFilter : public HTTPSourceFilter {
1616
public:
17+
[[nodiscard]] std::string_view getFilterName() const noexcept override {
18+
return "RequestContext";
19+
}
1720
explicit RequestContextFilter(
1821
std::shared_ptr<folly::RequestContext> context) noexcept
1922
: context_{std::move(context)} {

0 commit comments

Comments
 (0)