From 04a3628c771768eaef0967d7b15034c50da30691 Mon Sep 17 00:00:00 2001 From: Itamar Kaminski Date: Fri, 15 Jan 2021 18:49:13 +0200 Subject: [PATCH] Move resource data into OTLP 'Resource' Signed-off-by: Itamar Kaminski --- source/extensions/access_loggers/common/BUILD | 1 - .../common/grpc_access_logger.h | 1 - .../grpc/grpc_ot_access_log_impl.cc | 40 ++++++---- .../grpc/grpc_ot_access_log_impl.h | 8 +- .../grpc/grpc_ot_access_log_impl_test.cc | 77 +++++++++++++------ 5 files changed, 82 insertions(+), 45 deletions(-) diff --git a/source/extensions/access_loggers/common/BUILD b/source/extensions/access_loggers/common/BUILD index d7e449111640..e7851e9da725 100644 --- a/source/extensions/access_loggers/common/BUILD +++ b/source/extensions/access_loggers/common/BUILD @@ -34,7 +34,6 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/grpc:typed_async_client_lib", "//source/common/protobuf:utility_lib", - "//source/common/runtime:runtime_features_lib", "@com_google_absl//absl/types:optional", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], diff --git a/source/extensions/access_loggers/common/grpc_access_logger.h b/source/extensions/access_loggers/common/grpc_access_logger.h index 0f6f794dcdbd..f28b50f786be 100644 --- a/source/extensions/access_loggers/common/grpc_access_logger.h +++ b/source/extensions/access_loggers/common/grpc_access_logger.h @@ -12,7 +12,6 @@ #include "common/common/assert.h" #include "common/grpc/typed_async_client.h" #include "common/protobuf/utility.h" -#include "common/runtime/runtime_features.h" #include "absl/container/flat_hash_map.h" #include "absl/types/optional.h" diff --git a/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.cc b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.cc index 0936f5896099..8132733393d1 100644 --- a/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.cc +++ b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.cc @@ -30,17 +30,33 @@ GrpcOpenTelemetryAccessLoggerImpl::GrpcOpenTelemetryAccessLoggerImpl( Grpc::VersionedMethods("opentelemetry.proto.collector.logs.v1.LogsService.Export", "opentelemetry.proto.collector.logs.v1.LogsService.Export") .getMethodDescriptorForVersion(transport_api_version), - transport_api_version), - log_name_(log_name), local_info_(local_info) { - initRoot(); + transport_api_version) { + initMessageRoot(log_name, local_info); } +namespace { + +opentelemetry::proto::common::v1::KeyValue getStringKeyValue(const std::string& key, + const std::string& value) { + opentelemetry::proto::common::v1::KeyValue keyValue; + keyValue.set_key(key); + keyValue.mutable_value()->set_string_value(value); + return keyValue; +} + +} // namespace + // See comment about the structure of repeated fields in the header file. -void GrpcOpenTelemetryAccessLoggerImpl::initRoot() { - root_ = message_.add_resource_logs()->add_instrumentation_library_logs(); - // No reason to clear and refill these every time. - root_->mutable_instrumentation_library()->set_name("envoy"); - root_->mutable_instrumentation_library()->set_version("v1"); +// TODO(itamarkam): allow user configurable attributes. +void GrpcOpenTelemetryAccessLoggerImpl::initMessageRoot(const std::string& log_name, + const LocalInfo::LocalInfo& local_info) { + auto* resource_logs = message_.add_resource_logs(); + root_ = resource_logs->add_instrumentation_library_logs(); + auto* resource = resource_logs->mutable_resource(); + *resource->add_attributes() = getStringKeyValue("log_name", log_name); + *resource->add_attributes() = getStringKeyValue("zone_name", local_info.zoneName()); + *resource->add_attributes() = getStringKeyValue("cluster_name", local_info.clusterName()); + *resource->add_attributes() = getStringKeyValue("node_name", local_info.nodeName()); } void GrpcOpenTelemetryAccessLoggerImpl::addEntry( @@ -55,12 +71,8 @@ void GrpcOpenTelemetryAccessLoggerImpl::addEntry( bool GrpcOpenTelemetryAccessLoggerImpl::isEmpty() { return root_->logs().empty(); } -void GrpcOpenTelemetryAccessLoggerImpl::initMessage() { - // auto* resource = message_.mutable_resource_logs(0)->mutable_resource(); - // resource->add_attributes() .. - (void)log_name_; - (void)local_info_; -} +// The message is already initialized in the c'tor, and only the logs are cleared. +void GrpcOpenTelemetryAccessLoggerImpl::initMessage() {} void GrpcOpenTelemetryAccessLoggerImpl::clearMessage() { root_->clear_logs(); } diff --git a/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h index 5cb1b3563d18..4255b94fc963 100644 --- a/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h +++ b/source/extensions/access_loggers/grpc/grpc_ot_access_log_impl.h @@ -39,7 +39,7 @@ class GrpcOpenTelemetryAccessLoggerImpl envoy::config::core::v3::ApiVersion transport_api_version); private: - void initRoot(); + void initMessageRoot(const std::string& log_name, const LocalInfo::LocalInfo& local_info); // Extensions::AccessLoggers::GrpcCommon::GrpcAccessLogger void addEntry(opentelemetry::proto::logs::v1::LogRecord&& entry) override; void addEntry(opentelemetry::proto::logs::v1::ResourceLogs&& entry) override; @@ -47,8 +47,6 @@ class GrpcOpenTelemetryAccessLoggerImpl void initMessage() override; void clearMessage() override; - const std::string log_name_; - const LocalInfo::LocalInfo& local_info_; opentelemetry::proto::logs::v1::InstrumentationLibraryLogs* root_; }; @@ -57,10 +55,6 @@ class GrpcOpenTelemetryAccessLoggerCacheImpl GrpcOpenTelemetryAccessLoggerImpl, envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig> { public: - // Inherited constructor. - // using Common::GrpcAccessLoggerCache::GrpcAccessLoggerCache; GrpcOpenTelemetryAccessLoggerCacheImpl(Grpc::AsyncClientManager& async_client_manager, Stats::Scope& scope, ThreadLocal::SlotAllocator& tls, const LocalInfo::LocalInfo& local_info); diff --git a/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc b/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc index 2c29f0556669..be8fb5f12b56 100644 --- a/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc +++ b/test/extensions/access_loggers/grpc/grpc_ot_access_log_impl_test.cc @@ -21,11 +21,7 @@ using testing::_; using testing::Invoke; using testing::NiceMock; using testing::Return; - -// opentelemetry::proto::logs::v1::LogRecord, -// opentelemetry::proto::logs::v1::ResourceLogs /*TCP*/, -// opentelemetry::proto::collector::logs::v1::ExportLogsServiceRequest, -// opentelemetry::proto::collector::logs::v1::ExportLogsServiceResponse> +using testing::ReturnRef; namespace Envoy { namespace Extensions { @@ -35,6 +31,9 @@ namespace { constexpr std::chrono::milliseconds FlushInterval(10); constexpr int BUFFER_SIZE_BYTES = 0; +const std::string ZONE_NAME = "zone_name"; +const std::string CLUSTER_NAME = "cluster_name"; +const std::string NODE_NAME = "node_name"; // A helper test class to mock and intercept GrpcOpenTelemetryAccessLoggerImpl streams. class GrpcOpenTelemetryAccessLoggerImplTestHelper { @@ -45,8 +44,9 @@ class GrpcOpenTelemetryAccessLoggerImplTestHelper { GrpcOpenTelemetryAccessLoggerImplTestHelper(LocalInfo::MockLocalInfo& local_info, Grpc::MockAsyncClient* async_client) { - // EXPECT_CALL(local_info, node()); - (void)local_info; + EXPECT_CALL(local_info, zoneName()).WillOnce(ReturnRef(ZONE_NAME)); + EXPECT_CALL(local_info, clusterName()).WillOnce(ReturnRef(CLUSTER_NAME)); + EXPECT_CALL(local_info, nodeName()).WillOnce(ReturnRef(NODE_NAME)); EXPECT_CALL(*async_client, startRaw(_, _, _, _)) .WillOnce( Invoke([this](absl::string_view, absl::string_view, Grpc::RawAsyncStreamCallbacks& cbs, @@ -97,11 +97,22 @@ class GrpcOpenTelemetryAccessLoggerImplTest : public testing::Test { TEST_F(GrpcOpenTelemetryAccessLoggerImplTest, LogHttp) { grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( resource_logs: - - instrumentation_library_logs: - - instrumentation_library: - name: "envoy" - version: "v1" - logs: + resource: + attributes: + - key: "log_name" + value: + string_value: "test_log_name" + - key: "zone_name" + value: + string_value: "zone_name" + - key: "cluster_name" + value: + string_value: "cluster_name" + - key: "node_name" + value: + string_value: "node_name" + instrumentation_library_logs: + - logs: - severity_text: "test-severity-text" )EOF"); opentelemetry::proto::logs::v1::LogRecord entry; @@ -112,11 +123,22 @@ TEST_F(GrpcOpenTelemetryAccessLoggerImplTest, LogHttp) { TEST_F(GrpcOpenTelemetryAccessLoggerImplTest, LogTcp) { grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( resource_logs: - - instrumentation_library_logs: - - instrumentation_library: - name: "envoy" - version: "v1" - logs: + resource: + attributes: + - key: "log_name" + value: + string_value: "test_log_name" + - key: "zone_name" + value: + string_value: "zone_name" + - key: "cluster_name" + value: + string_value: "cluster_name" + - key: "node_name" + value: + string_value: "node_name" + instrumentation_library_logs: + - logs: - severity_text: "test-severity-text" )EOF"); opentelemetry::proto::logs::v1::LogRecord entry; @@ -161,11 +183,22 @@ TEST_F(GrpcOpenTelemetryAccessLoggerCacheImplTest, LoggerCreation) { logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP, scope_); grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF( resource_logs: - - instrumentation_library_logs: - - instrumentation_library: - name: "envoy" - version: "v1" - logs: + resource: + attributes: + - key: "log_name" + value: + string_value: "test-log" + - key: "zone_name" + value: + string_value: "zone_name" + - key: "cluster_name" + value: + string_value: "cluster_name" + - key: "node_name" + value: + string_value: "node_name" + instrumentation_library_logs: + - logs: - severity_text: "test-severity-text" )EOF"); opentelemetry::proto::logs::v1::LogRecord entry;