-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
tracing: Enable decorated operation in outbound (egress) listener to … #1858
Changes from 5 commits
60a3aa2
4bb6486
2a6d86d
31f5479
c996571
a73de8c
40b0b28
d129e89
accf425
43be090
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 |
---|---|---|
|
@@ -502,6 +502,11 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, | |
state_.saw_connection_close_ = true; | ||
} | ||
|
||
std::string decorator_operation; | ||
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. Instead of allocating a string here on the stack, it would be faster to just pass the pointer value of the header entry into the trace function as you did before. Any reason to switch it? 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 got a core dump when running the coverage tests - although strangely not when doing the bazel.dev build which also runs the tests. The problem seems to be caused by the header entry being removed/unallocated.
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 put the code back? If it fails I can help you figure out what the issue is. 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. Done |
||
if (request_headers_->EnvoyDecoratorOperation()) { | ||
decorator_operation = request_headers_->EnvoyDecoratorOperation()->value().c_str(); | ||
} | ||
|
||
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. so, there is some work going on in Istio to be able to decode a request using swagger spec/openapi spec.. And if that is the case, one might want to set the operation name based on the API spec. Would it make sense to have an interface for a filter to overwrite the operation name, in addition to the inbound header name doing the full override? 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 think that makes sense, but let's track in a separate issue? |
||
ConnectionManagerUtility::mutateRequestHeaders( | ||
*request_headers_, protocol, connection_manager_.read_callbacks_->connection(), | ||
connection_manager_.config_, *snapped_route_config_, connection_manager_.random_generator_, | ||
|
@@ -541,25 +546,52 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, | |
|
||
// Check if tracing is enabled at all. | ||
if (connection_manager_.config_.tracingConfig()) { | ||
Tracing::Decision tracing_decision = | ||
Tracing::HttpTracerUtility::isTracing(request_info_, *request_headers_); | ||
ConnectionManagerImpl::chargeTracingStats(tracing_decision.reason, | ||
connection_manager_.config_.tracingStats()); | ||
|
||
if (tracing_decision.is_tracing) { | ||
active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_); | ||
if (cached_route_.value() && cached_route_.value()->decorator()) { | ||
cached_route_.value()->decorator()->apply(*active_span_); | ||
} | ||
active_span_->injectContext(*request_headers_); | ||
} | ||
traceRequest(decorator_operation); | ||
} | ||
|
||
// Set the trusted address for the connection by taking the last address in XFF. | ||
request_info_.downstream_address_ = Utility::getLastAddressFromXFF(*request_headers_); | ||
decodeHeaders(nullptr, *request_headers_, end_stream); | ||
} | ||
|
||
void ConnectionManagerImpl::ActiveStream::traceRequest(const std::string& decorator_operation) { | ||
Tracing::Decision tracing_decision = | ||
Tracing::HttpTracerUtility::isTracing(request_info_, *request_headers_); | ||
ConnectionManagerImpl::chargeTracingStats(tracing_decision.reason, | ||
connection_manager_.config_.tracingStats()); | ||
|
||
if (!tracing_decision.is_tracing) { | ||
return; | ||
} | ||
|
||
active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_); | ||
|
||
// If a decorator has been defined, apply it to the active span | ||
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. super nit: please end sentences with '.'. Here and below. 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. should be sorted. |
||
if (cached_route_.value() && cached_route_.value()->decorator()) { | ||
cached_route_.value()->decorator()->apply(*active_span_); | ||
|
||
// For egress (outbound) requests, pass the decorator's operation name (if defined) | ||
// as a request header to enable the receiving service to use it in its server span | ||
if (connection_manager_.config_.tracingConfig()->operation_name_ == | ||
Tracing::OperationName::Egress && | ||
!cached_route_.value()->decorator()->getOperation().empty()) { | ||
request_headers_->insertEnvoyDecoratorOperation().value( | ||
cached_route_.value()->decorator()->getOperation()); | ||
} | ||
} | ||
|
||
// For igress (inbound) requests, if a decorator operation name has been provided, it | ||
// should be used to override the active span's operation | ||
if (!decorator_operation.empty() && | ||
connection_manager_.config_.tracingConfig()->operation_name_ == | ||
Tracing::OperationName::Ingress) { | ||
active_span_->setOperation(decorator_operation.c_str()); | ||
} | ||
|
||
// Inject the active span's tracing context into the request headers | ||
active_span_->injectContext(*request_headers_); | ||
} | ||
|
||
void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilter* filter, | ||
HeaderMap& headers, bool end_stream) { | ||
std::list<ActiveStreamDecoderFilterPtr>::iterator entry; | ||
|
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 this needs to be implemented? (Remove header on external requests, need to add test for that also in conn_manager_utility)