Skip to content

access log: add the detected protocol set #10524

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

Closed
kyessenov wants to merge 3 commits into from
Closed

access log: add the detected protocol set #10524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c1ce7b7
access log: add detected protocol set
kyessenov Mar 25, 2020
fecf6de
merge fix
kyessenov Mar 25, 2020
fb8cd06
debugging cruft
kyessenov Mar 25, 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
22 changes: 18 additions & 4 deletions include/envoy/stream_info/stream_info.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "common/singleton/const_singleton.h"

#include "absl/types/optional.h"
#include "absl/types/span.h"

namespace Envoy {

Expand Down Expand Up @@ -148,6 +149,18 @@ struct ResponseCodeDetailValues {

using ResponseCodeDetails = ConstSingleton<ResponseCodeDetailValues>;

struct ProtocolStringValues {
const std::string TcpString{"TCP"};
const std::string UdpString{"UDP"};
const std::string HttpString{"HTTP"};
const std::string Http10String{"HTTP/1.0"};
const std::string Http11String{"HTTP/1.1"};
const std::string Http2String{"HTTP/2"};
const std::string Http3String{"HTTP/3"};
};

using ProtocolStrings = ConstSingleton<ProtocolStringValues>;

struct UpstreamTiming {
/**
* Sets the time when the first byte of the request was sent upstream.
Expand Down Expand Up @@ -238,14 +251,14 @@ class StreamInfo {
virtual uint64_t bytesReceived() const PURE;

/**
* @return the protocol of the request.
* @return the set of protocols detected for a stream.
*/
virtual absl::optional<Http::Protocol> protocol() const PURE;
virtual absl::Span<const std::string> protocols() const PURE;

/**
* @param protocol the request's protocol.
* @param protocol the request's protocol
*/
virtual void protocol(Http::Protocol protocol) PURE;
virtual void addProtocol(absl::string_view protocol) PURE;

/**
* @return the response code.
Expand Down Expand Up @@ -486,6 +499,7 @@ class StreamInfo {
* filters (append only). Both object types can be consumed by multiple filters.
* @return the filter state associated with this request.
*/
virtual FilterStateSharedPtr& mutableFilterState() PURE;

@mattklein123 mattklein123 Mar 26, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It doesn't thrill me to expose a reference to the shared_ptr on the interface? Can you explain why this is needed?

@kyessenov kyessenov Mar 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

HTTP request stream info takes TCP stream info filter state by a mutable shared_ptr. I think an additional filter state was created before that was used for HTTP parent filter state, which does not feel right to me. I'm not sure why HTTP stream info is allowed to modify TCP stream info filter state.

virtual const FilterStateSharedPtr& filterState() PURE;
virtual const FilterState& filterState() const PURE;

Expand Down
13 changes: 8 additions & 5 deletions source/common/access_log/access_log_formatter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,15 @@ FormatterPtr AccessLogFormatUtils::defaultAccessLogFormatter() {
return FormatterPtr{new FormatterImpl(DEFAULT_FORMAT)};
}

const std::string&
AccessLogFormatUtils::protocolToString(const absl::optional<Http::Protocol>& protocol) {
if (protocol) {
const std::string AccessLogFormatUtils::protocolToString(absl::Span<const std::string> protocols) {
auto protocol = Http::Utility::getProtocol(protocols);
if (protocol.has_value()) {

@mattklein123 mattklein123 Mar 26, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need to do the string to enum to string dance here? Can't we just return the protocol string directly? Or this is for HTTP back compat? If for back compat can you add comments and also maybe skip the final getProtocolString call by returning the string reference directly from getProtocol?

@kyessenov kyessenov Mar 26, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Mostly for backwards compatibility. I can change the interface to return both the enum and the string.

@zuercher zuercher Apr 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think trying to mix strings and the protocol enum is going to be confusing and it already involves a lot of converting back and forth between the string and enum representation.

Perhaps a better approach would be to rename the existing StreamInfo::protocol() and StreamInfo::protocol(Http::Protocol) to http_protocol and provide a new StreamInfo::protocols() and StreamInfo::addProtocol that deal with strings. The implementation of http_protocol(Http::Protocol) would invoke addProtocol with the appropriate protocol name and also store the protocol in an absl::optional (returned by http_protocol()). That will make the changes in HCM simpler to review and allows access to the full stack of protocols in other situations.

Otherwise I think we'd need to work out a proper registry. I think that's feasible as long as we the identifiers are not exposed outside a given Envoy process (e.g. the X extension's ID may change from build-to-build but as long as the id is kept internal it shouldn't matter). We might even be able to arrange registrations of core protocols in a stable order so their ids wouldn't change from build-to-build.

@kyessenov kyessenov Apr 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it. I'll preserve protocol() for HTTP as-is and add two more stubs to deal with the layered protocols. Thank you! I can also do the rename separately (maybe there's some clang tool to automate that) to make reviews easier.

@mattklein123 mattklein123 Apr 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 this sounds like a good plan. Thank you!

return Http::Utility::getProtocolString(protocol.value());
}
return UnspecifiedValueString;
if (protocols.empty()) {
return UnspecifiedValueString;
}
return absl::StrJoin(protocols, ",");
}

const std::string AccessLogFormatUtils::getHostname() {
Expand Down Expand Up @@ -558,7 +561,7 @@ StreamInfoFormatter::StreamInfoFormatter(const std::string& field_name) {
} else if (field_name == "PROTOCOL") {
field_extractor_ = std::make_unique<StreamInfoStringFieldExtractor>(
[](const StreamInfo::StreamInfo& stream_info) {
return AccessLogFormatUtils::protocolToString(stream_info.protocol());
return AccessLogFormatUtils::protocolToString(stream_info.protocols());
});
} else if (field_name == "RESPONSE_CODE") {
field_extractor_ = std::make_unique<StreamInfoUInt64FieldExtractor>(
Expand Down
2 changes: 1 addition & 1 deletion source/common/access_log/access_log_formatter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class AccessLogFormatParser {
class AccessLogFormatUtils {
public:
static FormatterPtr defaultAccessLogFormatter();
static const std::string& protocolToString(const absl::optional<Http::Protocol>& protocol);
static const std::string protocolToString(absl::Span<const std::string> protocols);
static const std::string getHostname();

private:
Expand Down
10 changes: 7 additions & 3 deletions source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,9 @@ Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool
}

Network::FilterStatus ConnectionManagerImpl::onNewConnection() {
if (!read_callbacks_->connection().streamInfo().protocol()) {
read_callbacks_->connection().streamInfo().addProtocol(
StreamInfo::ProtocolStrings::get().HttpString);
if (!Http::Utility::getProtocol(read_callbacks_->connection().streamInfo().protocols())) {
// For Non-QUIC traffic, continue passing data to filters.
return Network::FilterStatus::Continue;
}
Expand Down Expand Up @@ -539,7 +541,7 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect
request_response_timespan_(new Stats::HistogramCompletableTimespanImpl(
connection_manager_.stats_.named_.downstream_rq_time_, connection_manager_.timeSource())),
stream_info_(connection_manager_.codec_->protocol(), connection_manager_.timeSource(),
connection_manager.filterState()),
connection_manager_.read_callbacks_->connection().streamInfo()),
upstream_options_(std::make_shared<Network::Socket::Options>()) {
ASSERT(!connection_manager.config_.isRoutable() ||
((connection_manager.config_.routeConfigProvider() == nullptr &&
Expand Down Expand Up @@ -814,7 +816,9 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
// HTTP/1.1.
//
// The protocol may have shifted in the HTTP/1.0 case so reset it.
stream_info_.protocol(protocol);
if (Http::Utility::getProtocol(stream_info_.protocols()) != protocol) {
stream_info_.addProtocol(Http::Utility::getProtocolString(protocol));

@mattklein123 mattklein123 Mar 26, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is going to change the behavior, right? Meaning we will now have multiple HTTP protocol strings?

@kyessenov kyessenov Mar 26, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is always appended and getProtocol looks from the back, so there is no change in the behavior. But we will have multiple strings in the protocol list as decisions are made about the protocol.

}
if (!connection_manager_.config_.http1Settings().accept_http_10_) {
// Send "Upgrade Required" if HTTP/1.0 support is not explicitly configured on.
sendLocalReply(false, Code::UpgradeRequired, "", nullptr, state_.is_head_request_,
Expand Down
4 changes: 0 additions & 4 deletions source/common/http/conn_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,6 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,

TimeSource& timeSource() { return time_source_; }

// Return a reference to the shared_ptr so that it can be lazy created on demand.
std::shared_ptr<StreamInfo::FilterState>& filterState() { return filter_state_; }

private:
struct ActiveStream;

Expand Down Expand Up @@ -787,7 +784,6 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
const Server::OverloadActionState& overload_stop_accepting_requests_ref_;
const Server::OverloadActionState& overload_disable_keepalive_ref_;
TimeSource& time_source_;
std::shared_ptr<StreamInfo::FilterState> filter_state_;

@kyessenov kyessenov Mar 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the filter state that was (lazily) created by a network filter. I changed it to use the filter state from the connection stream info. Otherwise, it's invisible to access loggers at the connection level.

};

} // namespace Http
Expand Down
7 changes: 0 additions & 7 deletions source/common/http/headers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,13 +271,6 @@ class HeaderValues {
const std::string True{"true"};
} CORSValues;

struct {
const std::string Http10String{"HTTP/1.0"};
const std::string Http11String{"HTTP/1.1"};
const std::string Http2String{"HTTP/2"};
const std::string Http3String{"HTTP/3"};
} ProtocolStrings;

struct {
const std::string Gzip{"gzip"};
const std::string Identity{"identity"};
Expand Down
24 changes: 20 additions & 4 deletions source/common/http/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,18 +636,34 @@ bool Utility::sanitizeConnectionHeader(Http::RequestHeaderMap& headers) {
const std::string& Utility::getProtocolString(const Protocol protocol) {
switch (protocol) {
case Protocol::Http10:
return Headers::get().ProtocolStrings.Http10String;
return StreamInfo::ProtocolStrings::get().Http10String;
case Protocol::Http11:
return Headers::get().ProtocolStrings.Http11String;
return StreamInfo::ProtocolStrings::get().Http11String;
case Protocol::Http2:
return Headers::get().ProtocolStrings.Http2String;
return StreamInfo::ProtocolStrings::get().Http2String;
case Protocol::Http3:
return Headers::get().ProtocolStrings.Http3String;
return StreamInfo::ProtocolStrings::get().Http3String;
}

NOT_REACHED_GCOVR_EXCL_LINE;
}

absl::optional<const Protocol> Utility::getProtocol(absl::Span<const std::string> protocols) {
for (auto it = protocols.rbegin(); it != protocols.rend(); it++) {

@mattklein123 mattklein123 Mar 26, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume you are going in reverse for a reason? Comment?

@kyessenov kyessenov Mar 26, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, sorry if it was not clear but it's basically append-only history of discovered protocols, so looking from the back is right.

if (*it == StreamInfo::ProtocolStrings::get().Http10String) {
return Protocol::Http10;
} else if (*it == StreamInfo::ProtocolStrings::get().Http11String) {
return Protocol::Http11;
} else if (*it == StreamInfo::ProtocolStrings::get().Http2String) {
return Protocol::Http2;
} else if (*it == StreamInfo::ProtocolStrings::get().Http3String) {
return Protocol::Http3;
}
}

return {};
}

void Utility::extractHostPathFromUri(const absl::string_view& uri, absl::string_view& host,
absl::string_view& path) {
/**
Expand Down
7 changes: 7 additions & 0 deletions source/common/http/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,13 @@ bool sanitizeConnectionHeader(Http::RequestHeaderMap& headers);
*/
const std::string& getProtocolString(const Protocol p);

/**
* Get the http protocol from the given protocol list.
* @param protocol list from the stream info.
* @return the last (most-specific) http protocol in the list.
*/
absl::optional<const Protocol> getProtocol(absl::Span<const std::string> protocols);

/**
* Extract host and path from a URI. The host may contain port.
* This function doesn't validate if the URI is valid. It only parses the URI with following
Expand Down
2 changes: 1 addition & 1 deletion source/common/router/header_formatter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ StreamInfoHeaderFormatter::StreamInfoHeaderFormatter(absl::string_view field_nam
: append_(append) {
if (field_name == "PROTOCOL") {
field_extractor_ = [](const Envoy::StreamInfo::StreamInfo& stream_info) {
return Envoy::AccessLog::AccessLogFormatUtils::protocolToString(stream_info.protocol());
return Envoy::AccessLog::AccessLogFormatUtils::protocolToString(stream_info.protocols());
};
} else if (field_name == "DOWNSTREAM_REMOTE_ADDRESS") {
field_extractor_ = [](const StreamInfo::StreamInfo& stream_info) {
Expand Down
3 changes: 2 additions & 1 deletion source/common/router/router.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
Http::ConnectionPool::Instance* Filter::getConnPool() {
// Choose protocol based on cluster configuration and downstream connection
// Note: Cluster may downgrade HTTP2 to HTTP1 based on runtime configuration.
Http::Protocol protocol = cluster_->upstreamHttpProtocol(callbacks_->streamInfo().protocol());
Http::Protocol protocol = cluster_->upstreamHttpProtocol(
Http::Utility::getProtocol(callbacks_->streamInfo().protocols()));
transport_socket_options_ = Network::TransportSocketOptionsUtility::fromFilterState(
*callbacks_->streamInfo().filterState());

Expand Down
1 change: 1 addition & 0 deletions source/common/stream_info/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ envoy_cc_library(
"//include/envoy/stream_info:stream_info_interface",
"//source/common/common:assert_lib",
"//source/common/common:dump_state_utils",
"//source/common/http:utility_lib",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
],
)
Expand Down
4 changes: 2 additions & 2 deletions source/common/stream_info/filter_state_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,8 @@ void FilterStateImpl::maybeCreateParent(ParentAccessMode parent_access_mode) {
if (life_span_ >= FilterState::LifeSpan::TopSpan) {
return;
}
if (absl::holds_alternative<std::shared_ptr<FilterState>>(ancestor_)) {
std::shared_ptr<FilterState> ancestor = absl::get<std::shared_ptr<FilterState>>(ancestor_);
if (absl::holds_alternative<FilterStateSharedPtr>(ancestor_)) {
FilterStateSharedPtr ancestor = absl::get<FilterStateSharedPtr>(ancestor_);
if (ancestor == nullptr || ancestor->lifeSpan() != life_span_ + 1) {
parent_ = std::make_shared<FilterStateImpl>(ancestor, FilterState::LifeSpan(life_span_ + 1));
} else {
Expand Down
10 changes: 5 additions & 5 deletions source/common/stream_info/filter_state_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ class FilterStateImpl : public FilterState {
* @param ancestor a std::shared_ptr storing an already created ancestor.
* @param life_span the life span this is handling.
*/
FilterStateImpl(std::shared_ptr<FilterState> ancestor, FilterState::LifeSpan life_span)
FilterStateImpl(FilterStateSharedPtr ancestor, FilterState::LifeSpan life_span)
: ancestor_(ancestor), life_span_(life_span) {
maybeCreateParent(ParentAccessMode::ReadOnly);
}

using LazyCreateAncestor = std::pair<std::shared_ptr<FilterState>&, FilterState::LifeSpan>;
using LazyCreateAncestor = std::pair<FilterStateSharedPtr&, FilterState::LifeSpan>;
/**
* @param ancestor a std::pair storing an ancestor, that can be passed in as a way to lazy
* initialize a FilterState that's owned by an object with bigger scope than this. This is to
Expand All @@ -49,7 +49,7 @@ class FilterStateImpl : public FilterState {
bool hasDataAtOrAboveLifeSpan(FilterState::LifeSpan life_span) const override;

FilterState::LifeSpan lifeSpan() const override { return life_span_; }
std::shared_ptr<FilterState> parent() const override { return parent_; }
FilterStateSharedPtr parent() const override { return parent_; }

private:
// This only checks the local data_storage_ for data_name existence.
Expand All @@ -62,8 +62,8 @@ class FilterStateImpl : public FilterState {
FilterState::StateType state_type_;
};

absl::variant<std::shared_ptr<FilterState>, LazyCreateAncestor> ancestor_;
std::shared_ptr<FilterState> parent_;
absl::variant<FilterStateSharedPtr, LazyCreateAncestor> ancestor_;
FilterStateSharedPtr parent_;
const FilterState::LifeSpan life_span_;
absl::flat_hash_map<std::string, std::unique_ptr<FilterObject>> data_storage_;
};
Expand Down
30 changes: 20 additions & 10 deletions source/common/stream_info/stream_info_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include "common/common/assert.h"
#include "common/common/dump_state_utils.h"
#include "common/http/utility.h"
#include "common/stream_info/filter_state_impl.h"

namespace Envoy {
Expand All @@ -23,17 +24,22 @@ struct StreamInfoImpl : public StreamInfo {

StreamInfoImpl(Http::Protocol protocol, TimeSource& time_source)
: time_source_(time_source), start_time_(time_source.systemTime()),
start_time_monotonic_(time_source.monotonicTime()), protocol_(protocol),
start_time_monotonic_(time_source.monotonicTime()),
protocols_({Http::Utility::getProtocolString(protocol)}),
filter_state_(std::make_shared<FilterStateImpl>(FilterState::LifeSpan::FilterChain)) {}

StreamInfoImpl(Http::Protocol protocol, TimeSource& time_source,
std::shared_ptr<FilterState>& parent_filter_state)
StreamInfoImpl(Http::Protocol protocol, TimeSource& time_source, StreamInfo& parent)
: time_source_(time_source), start_time_(time_source.systemTime()),
start_time_monotonic_(time_source.monotonicTime()), protocol_(protocol),
start_time_monotonic_(time_source.monotonicTime()),
filter_state_(std::make_shared<FilterStateImpl>(
FilterStateImpl::LazyCreateAncestor(parent_filter_state,
FilterStateImpl::LazyCreateAncestor(parent.mutableFilterState(),
FilterState::LifeSpan::DownstreamConnection),
FilterState::LifeSpan::FilterChain)) {}
FilterState::LifeSpan::FilterChain)) {
for (const auto& protocol : parent.protocols()) {
protocols_.push_back(protocol);
}
protocols_.push_back(Http::Utility::getProtocolString(protocol));
}

SystemTime startTime() const override { return start_time_; }

Expand Down Expand Up @@ -108,9 +114,11 @@ struct StreamInfoImpl : public StreamInfo {

uint64_t bytesReceived() const override { return bytes_received_; }

absl::optional<Http::Protocol> protocol() const override { return protocol_; }
absl::Span<const std::string> protocols() const override { return protocols_; }

void protocol(Http::Protocol protocol) override { protocol_ = protocol; }
void addProtocol(absl::string_view protocol) override {
protocols_.push_back(std::string(protocol));
}

absl::optional<uint32_t> responseCode() const override { return response_code_; }

Expand Down Expand Up @@ -216,6 +224,7 @@ struct StreamInfoImpl : public StreamInfo {
(*metadata_.mutable_filter_metadata())[name].MergeFrom(value);
};

FilterStateSharedPtr& mutableFilterState() override { return filter_state_; }
const FilterStateSharedPtr& filterState() override { return filter_state_; }
const FilterState& filterState() const override { return *filter_state_; }

Expand Down Expand Up @@ -248,7 +257,8 @@ struct StreamInfoImpl : public StreamInfo {

void dumpState(std::ostream& os, int indent_level = 0) const {
const char* spaces = spacesForLevel(indent_level);
os << spaces << "StreamInfoImpl " << this << DUMP_OPTIONAL_MEMBER(protocol_)
os << spaces << "StreamInfoImpl " << this
<< DUMP_OPTIONAL_MEMBER(Http::Utility::getProtocol(protocols_))
<< DUMP_OPTIONAL_MEMBER(response_code_) << DUMP_OPTIONAL_MEMBER(response_code_details_)
<< DUMP_MEMBER(health_check_request_) << DUMP_MEMBER(route_name_) << "\n";
}
Expand All @@ -271,7 +281,7 @@ struct StreamInfoImpl : public StreamInfo {
absl::optional<MonotonicTime> last_downstream_tx_byte_sent_;
absl::optional<MonotonicTime> final_time_;

absl::optional<Http::Protocol> protocol_;
std::vector<std::string> protocols_;

@mattklein123 mattklein123 Mar 26, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would recommend an InlinedVector here with a small number of protocols.

@kyessenov kyessenov Mar 26, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I am relying on inlined strings to save some space.

absl::optional<uint32_t> response_code_;
absl::optional<std::string> response_code_details_;
uint64_t response_flags_{};
Expand Down
4 changes: 2 additions & 2 deletions source/common/tracing/http_tracer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ void HttpTracerUtility::finalizeDownstreamSpan(Span& span,
valueOrDefault(request_headers->EnvoyDownstreamServiceCluster(), "-"));
span.setTag(Tracing::Tags::get().UserAgent, valueOrDefault(request_headers->UserAgent(), "-"));
span.setTag(Tracing::Tags::get().HttpProtocol,
AccessLog::AccessLogFormatUtils::protocolToString(stream_info.protocol()));
AccessLog::AccessLogFormatUtils::protocolToString(stream_info.protocols()));

const auto& remote_address = stream_info.downstreamDirectRemoteAddress();

Expand Down Expand Up @@ -215,7 +215,7 @@ void HttpTracerUtility::finalizeUpstreamSpan(Span& span,
const StreamInfo::StreamInfo& stream_info,
const Config& tracing_config) {
span.setTag(Tracing::Tags::get().HttpProtocol,
AccessLog::AccessLogFormatUtils::protocolToString(stream_info.protocol()));
AccessLog::AccessLogFormatUtils::protocolToString(stream_info.protocols()));

if (stream_info.upstreamHost()) {
span.setTag(Tracing::Tags::get().UpstreamAddress,
Expand Down
1 change: 1 addition & 0 deletions source/extensions/access_loggers/grpc/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ envoy_cc_library(
deps = [
":grpc_access_log_lib",
":grpc_access_log_utils",
"//source/common/http:utility_lib",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
"@envoy_api//envoy/data/accesslog/v3:pkg_cc_proto",
"@envoy_api//envoy/extensions/access_loggers/grpc/v3:pkg_cc_proto",
Expand Down
Loading