xray: Use correct types for segment document output #10834
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| #include "common/common/fmt.h" | ||
| #include "common/common/hex.h" | ||
| #include "common/protobuf/message_validator_impl.h" | ||
| #include "common/protobuf/utility.h" | ||
| #include "common/runtime/runtime_impl.h" | ||
|
|
||
| #include "source/extensions/tracers/xray/daemon.pb.validate.h" | ||
|
|
@@ -76,17 +77,28 @@ void Span::finishSpan() { | |
| s.set_end_time( | ||
| time_point_cast<SecondsWithFraction>(time_source_.systemTime()).time_since_epoch().count()); | ||
| s.set_parent_id(parentId()); | ||
|
|
||
| // HTTP annotations | ||
| using StructField = Protobuf::MapPair<std::string, ProtobufWkt::Value>; | ||
| daemon::Segment_http_annotations* http = s.mutable_http(); | ||
|
|
||
| ProtobufWkt::Struct* request = http->mutable_request(); | ||
|
There was a problem hiding this comment. I'd change those two lines to: ProtobufWkt::Struct* request = s.http()->mutable_request();The There was a problem hiding this comment. makes sense. this was left over from my misunderstanding that constructing submessages and assigning isn't the right way to go in c++. |
||
| auto request_fields = request->mutable_fields(); | ||
|
There was a problem hiding this comment. nit: |
||
| for (const auto& field : http_request_annotations_) { | ||
| request_fields->insert(StructField{field.first, field.second}); | ||
|
|
||
| (*request_fields)[field.first] = field.second; | ||
|
There was a problem hiding this comment. This line is a duplicate. remove? There was a problem hiding this comment. bleh, looking at this and cout it looks like I didn't look at my rebase merge closely enough. fixed. |
||
| } | ||
|
|
||
| ProtobufWkt::Struct* response = http->mutable_response(); | ||
| auto response_fields = response->mutable_fields(); | ||
| for (const auto& field : http_response_annotations_) { | ||
| response_fields->insert(StructField{field.first, field.second}); | ||
| } | ||
|
|
||
| using KeyValue = Protobuf::Map<std::string, std::string>::value_type; | ||
| for (const auto& item : custom_annotations_) { | ||
| s.mutable_annotations()->insert(KeyValue{item.first, item.second}); | ||
| } | ||
|
|
||
| const std::string json = MessageUtil::getJsonStringFromMessage( | ||
|
|
@@ -179,20 +191,31 @@ void Span::setTag(absl::string_view name, absl::string_view value) { | |
| } | ||
|
|
||
| if (name == HttpUrl) { | ||
| http_request_annotations_.emplace(SpanUrl, ValueUtil::stringValue(std::string(value))); | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. So value is a string_view, and based off my read of the abseil docs and the header there is no implicit conversion from string_view to string. As rusty as I am at c++, I'm assuming the reason the "implicit conversion" works for flat_hash_map::emplace is because its constructing from arguments internally There was a problem hiding this comment. if value is a string_view, then this is fine. |
||
| } else if (name == HttpMethod) { | ||
| http_request_annotations_.emplace(SpanMethod, ValueUtil::stringValue(std::string(value))); | ||
| } else if (name == HttpUserAgent) { | ||
| http_request_annotations_.emplace(SpanUserAgent, ValueUtil::stringValue(std::string(value))); | ||
| } else if (name == HttpStatusCode) { | ||
| uint64_t status_code; | ||
| std::cout << "belldav" << value << std::endl; | ||
|
|
||
| if (!absl::SimpleAtoi(value, &status_code)) { | ||
| ENVOY_LOG(warn, "{} must be a number, given: {}", HttpStatusCode, value); | ||
| return; | ||
| } | ||
|
There was a problem hiding this comment. Rather than discard the entire trace, it's better just to skip the status if we can't parse it. Also, Matt is likely to have you change this log verbosity to debug or trace. There was a problem hiding this comment. setTag only operates on a single key-value pair, so this won't fail the entire trace. I could however see failing to parse falling back to registering a custom annotation with these values. There was a problem hiding this comment. You're right. I confused this with the code in finishSpan(). There was a problem hiding this comment. I did drop the log level down to debug. |
||
| http_response_annotations_.emplace(SpanStatus, ValueUtil::numberValue(status_code)); | ||
| } else if (name == HttpResponseSize) { | ||
| uint64_t response_size; | ||
| if (!absl::SimpleAtoi(value, &response_size)) { | ||
| ENVOY_LOG(warn, "{} must be a number, given: {}", HttpResponseSize, value); | ||
| return; | ||
| } | ||
| http_response_annotations_.emplace(SpanContentLength, ValueUtil::numberValue(response_size)); | ||
|
There was a problem hiding this comment.
|
||
| } else if (name == PeerAddress) { | ||
| http_request_annotations_.emplace(SpanClientIp, ValueUtil::stringValue(std::string(value))); | ||
| // In this case, PeerAddress refers to the client's actual IP address, not | ||
| // the address specified in the the HTTP X-Forwarded-For header. | ||
| http_request_annotations_.emplace(SpanXForwardedFor, ValueUtil::boolValue(false)); | ||
| } else { | ||
| custom_annotations_.emplace(name, value); | ||
| } | ||
|
|
||
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.
There's already a
numberValue(..)utility function on line 459.You can remove this function and its definition.
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.
removed.