Skip to content
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.

Sign up for GitHub

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

xray: Use correct types for segment document output #10834

Merged
merged 7 commits into from
Apr 22, 2020
Merged

xray: Use correct types for segment document output #10834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ecd5844
utility: Add numberValue constructor proto utility functions
Apr 17, 2020
91ab2b1
xray: Convert http request/response annotations to proto3 structs to …
Apr 17, 2020
a9c6aa6
clang is less permissive about captures than gcc
Apr 20, 2020
239b76a
review comments
Apr 21, 2020
39c8e05
Revert "utility: Add numberValue constructor proto utility functions"
Apr 21, 2020
79ffe41
drop log level to debug
Apr 21, 2020
b3b9cf5
drop redundant insert
Apr 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions source/extensions/tracers/xray/daemon.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ syntax = "proto3";
package source.extensions.tracers.xray.daemon;

import "validate/validate.proto";
import "google/protobuf/struct.proto";

// see https://docs.aws.amazon.com/xray/latest/devguide/xray-api-segmentdocuments.html
message Segment {
Expand All @@ -14,12 +15,12 @@ message Segment {
double start_time = 4 [(validate.rules).double = {gt: 0}];
double end_time = 5 [(validate.rules).double = {gt: 0}];
string parent_id = 6;
map<string, string> annotations = 7;
http_annotations http = 8;
http_annotations http = 7;
message http_annotations {
map<string, string> request = 1;
map<string, string> response = 2;
google.protobuf.Struct request = 1;
google.protobuf.Struct response = 2;
}
map<string, string> annotations = 8;
}

message Header {
Expand Down
47 changes: 33 additions & 14 deletions source/extensions/tracers/xray/tracer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -76,17 +77,25 @@ void Span::finishSpan() {
s.set_end_time(
time_point_cast<SecondsWithFraction>(time_source_.systemTime()).time_since_epoch().count());
s.set_parent_id(parentId());
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});

// HTTP annotations
using StructField = Protobuf::MapPair<std::string, ProtobufWkt::Value>;

ProtobufWkt::Struct* request = s.mutable_http()->mutable_request();
auto* request_fields = request->mutable_fields();
for (const auto& field : http_request_annotations_) {
request_fields->insert(StructField{field.first, field.second});
}

for (const auto& item : http_request_annotations_) {
s.mutable_http()->mutable_request()->insert(KeyValue{item.first, item.second});
ProtobufWkt::Struct* response = s.mutable_http()->mutable_response();
auto* response_fields = response->mutable_fields();
for (const auto& field : http_response_annotations_) {
response_fields->insert(StructField{field.first, field.second});
}

for (const auto& item : http_response_annotations_) {
s.mutable_http()->mutable_response()->insert(KeyValue{item.first, item.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(
Expand Down Expand Up @@ -179,20 +188,30 @@ void Span::setTag(absl::string_view name, absl::string_view value) {
}

if (name == HttpUrl) {
http_request_annotations_.emplace(SpanUrl, value);
http_request_annotations_.emplace(SpanUrl, ValueUtil::stringValue(std::string(value)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value is a string already.
so this should be ValueUtil::stringValue(value); instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if value is a string_view, then this is fine.

} else if (name == HttpMethod) {
http_request_annotations_.emplace(SpanMethod, value);
http_request_annotations_.emplace(SpanMethod, ValueUtil::stringValue(std::string(value)));
} else if (name == HttpUserAgent) {
http_request_annotations_.emplace(SpanUserAgent, value);
http_request_annotations_.emplace(SpanUserAgent, ValueUtil::stringValue(std::string(value)));
} else if (name == HttpStatusCode) {
http_response_annotations_.emplace(SpanStatus, value);
uint64_t status_code;
if (!absl::SimpleAtoi(value, &status_code)) {
ENVOY_LOG(debug, "{} must be a number, given: {}", HttpStatusCode, value);
return;
}
http_response_annotations_.emplace(SpanStatus, ValueUtil::numberValue(status_code));
} else if (name == HttpResponseSize) {
http_response_annotations_.emplace(SpanContentLength, value);
uint64_t response_size;
if (!absl::SimpleAtoi(value, &response_size)) {
ENVOY_LOG(debug, "{} must be a number, given: {}", HttpResponseSize, value);
return;
}
http_response_annotations_.emplace(SpanContentLength, ValueUtil::numberValue(response_size));
} else if (name == PeerAddress) {
http_request_annotations_.emplace(SpanClientIp, value);
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, "false");
http_request_annotations_.emplace(SpanXForwardedFor, ValueUtil::boolValue(false));
} else {
custom_annotations_.emplace(name, value);
}
Expand Down
7 changes: 4 additions & 3 deletions source/extensions/tracers/xray/tracer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "envoy/tracing/http_tracer.h"

#include "common/common/hex.h"
#include "common/protobuf/utility.h"

#include "extensions/tracers/xray/daemon_broker.h"
#include "extensions/tracers/xray/sampling_strategy.h"
Expand All @@ -23,7 +24,7 @@ namespace XRay {

constexpr auto XRayTraceHeader = "x-amzn-trace-id";

class Span : public Tracing::Span {
class Span : public Tracing::Span, Logger::Loggable<Logger::Id::config> {
public:
/**
* Creates a new Span.
Expand Down Expand Up @@ -147,8 +148,8 @@ class Span : public Tracing::Span {
std::string trace_id_;
std::string parent_segment_id_;
std::string name_;
absl::flat_hash_map<std::string, std::string> http_request_annotations_;
absl::flat_hash_map<std::string, std::string> http_response_annotations_;
absl::flat_hash_map<std::string, ProtobufWkt::Value> http_request_annotations_;
absl::flat_hash_map<std::string, ProtobufWkt::Value> http_response_annotations_;
absl::flat_hash_map<std::string, std::string> custom_annotations_;
Envoy::TimeSource& time_source_;
DaemonBroker& broker_;
Expand Down
33 changes: 20 additions & 13 deletions test/extensions/tracers/xray/tracer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "test/mocks/server/mocks.h"
#include "test/mocks/tracing/mocks.h"

#include "absl/strings/str_format.h"
#include "absl/strings/str_split.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
Expand Down Expand Up @@ -46,13 +47,13 @@ TEST_F(XRayTracerTest, SerializeSpanTest) {
constexpr auto expected_http_method = "POST";
constexpr auto expected_http_url = "/first/second";
constexpr auto expected_user_agent = "Mozilla/5.0 (Macintosh; Intel Mac OS X)";
constexpr auto expected_status_code = "202";
constexpr auto expected_content_length = "1337";
constexpr uint32_t expected_status_code = 202;
constexpr uint32_t expected_content_length = 1337;
constexpr auto expected_client_ip = "10.0.0.100";
constexpr auto expected_x_forwarded_for = "false";
constexpr auto expected_x_forwarded_for = false;
constexpr auto expected_upstream_address = "10.0.0.200";

auto on_send = [](const std::string& json) {
auto on_send = [&](const std::string& json) {
ASSERT_FALSE(json.empty());
daemon::Segment s;
MessageUtil::loadFromJson(json, s, ProtobufMessage::getNullValidationVisitor());
Expand All @@ -61,13 +62,19 @@ TEST_F(XRayTracerTest, SerializeSpanTest) {
ASSERT_EQ(1, s.annotations().size());
ASSERT_TRUE(s.parent_id().empty());
ASSERT_STREQ(expected_span_name, s.name().c_str());
ASSERT_STREQ(expected_http_method, s.http().request().at("method").c_str());
ASSERT_STREQ(expected_http_url, s.http().request().at("url").c_str());
ASSERT_STREQ(expected_user_agent, s.http().request().at("user_agent").c_str());
ASSERT_STREQ(expected_status_code, s.http().response().at("status").c_str());
ASSERT_STREQ(expected_content_length, s.http().response().at("content_length").c_str());
ASSERT_STREQ(expected_client_ip, s.http().request().at("client_ip").c_str());
ASSERT_STREQ(expected_x_forwarded_for, s.http().request().at("x_forwarded_for").c_str());
ASSERT_STREQ(expected_http_method,
s.http().request().fields().at("method").string_value().c_str());
ASSERT_STREQ(expected_http_url, s.http().request().fields().at("url").string_value().c_str());
ASSERT_STREQ(expected_user_agent,
s.http().request().fields().at("user_agent").string_value().c_str());
ASSERT_DOUBLE_EQ(expected_status_code,
s.http().response().fields().at("status").number_value());
ASSERT_DOUBLE_EQ(expected_content_length,
s.http().response().fields().at("content_length").number_value());
ASSERT_STREQ(expected_client_ip,
s.http().request().fields().at("client_ip").string_value().c_str());
ASSERT_EQ(expected_x_forwarded_for,
s.http().request().fields().at("x_forwarded_for").bool_value());
ASSERT_STREQ(expected_upstream_address, s.annotations().at("upstream_address").c_str());
};

Expand All @@ -78,8 +85,8 @@ TEST_F(XRayTracerTest, SerializeSpanTest) {
span->setTag("http.method", expected_http_method);
span->setTag("http.url", expected_http_url);
span->setTag("user_agent", expected_user_agent);
span->setTag("http.status_code", expected_status_code);
span->setTag("response_size", expected_content_length);
span->setTag("http.status_code", absl::StrFormat("%d", expected_status_code));
span->setTag("response_size", absl::StrFormat("%d", expected_content_length));
span->setTag("peer.address", expected_client_ip);
span->setTag("upstream_address", expected_upstream_address);
span->finishSpan();
Expand Down