Tracing: populate tags based on the custom headers#690
Conversation
| are the only supported values. | ||
|
|
||
| request_headers_for_tags | ||
| *(optional, array)* An optional list of header names which is used for populating tags the an active span. |
There was a problem hiding this comment.
last part of the sentence doesn't make sense.
There was a problem hiding this comment.
yeah, 'on' is missing
| are the only supported values. | ||
|
|
||
| request_headers_for_tags | ||
| *(optional, array)* An optional list of header names which is used for populating tags on the an active span. |
| request_headers_for_tags | ||
| *(optional, array)* An optional list of header names which is used for populating tags on the an active span. | ||
| Each tag name is the header name and tag value is the header value from the request headers. | ||
| If specified header is not present in the request headers no tag is created. |
include/envoy/http/header_map.h
Outdated
|
|
||
| const std::string& get() const { return string_; } | ||
| bool operator==(const LowerCaseString& rhs) const { return string_ == rhs.string_; } | ||
| LowerCaseString& operator=(const LowerCaseString& rhs) { |
There was a problem hiding this comment.
should not be needed and is probably indicative of doing something wrong.
There was a problem hiding this comment.
that's because Optional does not have a support for moveable types. Would you suggest to add it there?
There was a problem hiding this comment.
I don't know what the actual problem you are trying to solve is, but I think you shouldn't need any of it, so unclear on the actual problem.
There was a problem hiding this comment.
as far as i can tell the following happens:
Optional.value(arg) method does copy assignment for the argument passed in, what ends up happening vector is copy assigned and consequently LowerCaseString is copy assigned which is not permitted as we have move ctor for the LowerCaseString and compiler removes implicit operator=(const LowerCaseString&).
That's why one solution is to explicitly define copy assignment.
There was a problem hiding this comment.
per offline convo make config a pointer to a struct and remove operator= from here (so we don't introduce accidental copies).
include/envoy/tracing/http_tracer.h
Outdated
| virtual ~Config() {} | ||
|
|
||
| /** | ||
| * Operation name for tracing, e.g., ingress. |
include/envoy/tracing/http_tracer.h
Outdated
| virtual OperationName operationName() const PURE; | ||
|
|
||
| /** | ||
| * List of headers to populate tags on the active span. |
| if (request_info_.healthCheck()) { | ||
| connection_manager_.config_.tracingStats().health_check_.inc(); | ||
| } else { | ||
| Tracing::HttpTracerUtility::populateTagsBasedOnHeaders(*active_span_, *request_headers_, |
There was a problem hiding this comment.
just move this logic into finalizeSpan
| */ | ||
| struct TracingConnectionManagerConfig { | ||
| Tracing::OperationName operation_name_; | ||
| std::list<Http::LowerCaseString> request_headers_for_tags_; |
| are the only supported values. | ||
|
|
||
| request_headers_for_tags | ||
| *(optional, array)* An optional list of header names which is used for populating tags on the active span. |
There was a problem hiding this comment.
still multiple grammar issues. Please pair write with someone.
There was a problem hiding this comment.
will do tomorrow morning
3 participants
No description provided.