-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Egress support v2 #157
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
Egress support v2 #157
Changes from 9 commits
e16e6f9
993c4f4
d6a5321
2d4edd1
6b6b391
2ab45ca
5ade1b5
a4d013b
851238e
499b204
d41363b
88545df
eef004f
264600d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,13 @@ | |
|
|
||
| namespace Tracing { | ||
|
|
||
| class TracingContext { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docs
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| public: | ||
| virtual ~TracingContext() {} | ||
|
|
||
| virtual const std::string& operationName() const PURE; | ||
| }; | ||
|
|
||
| /** | ||
| * Http sink for traces. Sink is responsible for delivering trace to the collector. | ||
| */ | ||
|
|
@@ -15,7 +22,8 @@ class HttpSink { | |
|
|
||
| virtual void flushTrace(const Http::HeaderMap& request_headers, | ||
| const Http::HeaderMap& response_headers, | ||
| const Http::AccessLog::RequestInfo& request_info) PURE; | ||
| const Http::AccessLog::RequestInfo& request_info, | ||
| const TracingContext& tracing_context) PURE; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<HttpSink> HttpSinkPtr; | ||
|
|
@@ -30,7 +38,8 @@ class HttpTracer { | |
| virtual void addSink(HttpSinkPtr&& sink) PURE; | ||
| virtual void trace(const Http::HeaderMap* request_headers, | ||
| const Http::HeaderMap* response_headers, | ||
| const Http::AccessLog::RequestInfo& request_info) PURE; | ||
| const Http::AccessLog::RequestInfo& request_info, | ||
| const TracingContext& tracing_context) PURE; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<HttpTracer> HttpTracerPtr; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,23 @@ struct ConnectionManagerStats { | |
| Stats::Store& store_; | ||
| }; | ||
|
|
||
| enum class TracingType { | ||
| // Trace all traceable requests. | ||
| All, | ||
| // Trace only when there is an upstream failure reason. | ||
| UpstreamFailureReason | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would call this UpstreamFailure
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| }; | ||
|
|
||
| /** | ||
| * Configuration for tracing which is set on the connection manager level. | ||
| * Http Tracing can be enabled/disabled on a per connection manager basis. | ||
| * Here we specify some specific for connection manager settings. | ||
| */ | ||
| struct TracingConnectionManagerConfig { | ||
| std::string operation_name_; | ||
| TracingType tracing_type_; | ||
| }; | ||
|
|
||
| /** | ||
| * Abstract configuration for the connection manager. | ||
| */ | ||
|
|
@@ -159,9 +176,9 @@ class ConnectionManagerConfig { | |
| virtual const Optional<std::string>& userAgent() PURE; | ||
|
|
||
| /** | ||
| * @return true if tracing is enabled otherwise it returns false; | ||
| * @return tracing config. | ||
| */ | ||
| virtual bool isTracing() PURE; | ||
| virtual const Optional<TracingConnectionManagerConfig>& tracingConfig() PURE; | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -319,7 +336,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| public Event::DeferredDeletable, | ||
| public StreamCallbacks, | ||
| public StreamDecoder, | ||
| public FilterChainFactoryCallbacks { | ||
| public FilterChainFactoryCallbacks, | ||
| public Tracing::TracingContext { | ||
| ActiveStream(ConnectionManagerImpl& connection_manager); | ||
| ~ActiveStream(); | ||
|
|
||
|
|
@@ -349,6 +367,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| void addStreamEncoderFilter(StreamEncoderFilterPtr filter) override; | ||
| void addStreamFilter(StreamFilterPtr filter) override; | ||
|
|
||
| // Tracing::TracingContext | ||
| virtual const std::string& operationName() const override; | ||
|
|
||
| static DateFormatter date_formatter_; | ||
|
|
||
| // All state for the stream. Put here for readability. We could move this to a bit field | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,7 +104,7 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea | |
| } | ||
| } | ||
|
|
||
| if (config.isTracing()) { | ||
| if (config.tracingConfig().valid()) { | ||
| Tracing::HttpTracerUtility::mutateHeaders(request_headers, runtime); | ||
| } | ||
| } | ||
|
|
@@ -131,4 +131,22 @@ void ConnectionManagerUtility::mutateResponseHeaders(Http::HeaderMap& response_h | |
| } | ||
| } | ||
|
|
||
| bool ConnectionManagerUtility::shouldTraceRequest( | ||
| const Http::AccessLog::RequestInfo& request_info, | ||
| const Optional<TracingConnectionManagerConfig>& config) { | ||
| if (!config.valid()) { | ||
| return false; | ||
| } | ||
|
|
||
| switch (config.value().tracing_type_) { | ||
| case Http::TracingType::All: | ||
| return true; | ||
| case Http::TracingType::UpstreamFailureReason: | ||
| return request_info.failureReason() != Http::AccessLog::FailureReason::None; | ||
| } | ||
|
|
||
| // This should not happen as switch above covers all cases, this is just to make compiler happy. | ||
| throw EnvoyException("unknown tracing type"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add the tracing_type value to the exception?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just use NOT_IMPLEMENTED or something clear that it should not happen
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'll switch to NOT_IMPLEMENTED here and refine the comment to emphasize that compiler also enforces all types covered by switch above. |
||
| } | ||
|
|
||
| } // Http | ||
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.
generally a sub-level json object gets its own section (see other docs). Can you make a new section on this page, put the nested json docs there, and link to it from here (again see other docs)