Skip to content

Commit

Permalink
Reviewer comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@yashykt
yashykt committed Jan 30, 2025
1 parent 8eaa011 commit 0860652
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 100 deletions.
69 changes: 23 additions & 46 deletions src/cpp/ext/otel/otel_plugin.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,16 +146,6 @@ class OpenTelemetryPluginImpl::NPCMetricsKeyValueIterable
const OptionalLabelsBitSet& optional_labels_bits_;
};

absl::string_view NoStdStringViewToAbslStringView(
opentelemetry::nostd::string_view string) {
return absl::string_view(string.data(), string.size());
}

opentelemetry::nostd::string_view AbslStringViewToNoStdStringView(
absl::string_view string) {
return opentelemetry::nostd::string_view(string.data(), string.size());
}

//
// OpenTelemetryPluginBuilderImpl
//
Expand Down Expand Up @@ -400,6 +390,14 @@ void OpenTelemetryPluginImpl::ServerBuilderOption::UpdateArguments(
plugin_->AddToChannelArguments(args);
}

namespace {
opentelemetry::nostd::shared_ptr<opentelemetry::trace::Tracer> MaybeMakeTracer(
opentelemetry::trace::TracerProvider* tracer_provider) {
if (tracer_provider == nullptr) return nullptr;
return tracer_provider->GetTracer("grpc-c++", GRPC_CPP_VERSION_STRING);
}
} // namespace

OpenTelemetryPluginImpl::OpenTelemetryPluginImpl(
const absl::flat_hash_set<std::string>& metrics,
opentelemetry::nostd::shared_ptr<opentelemetry::metrics::MeterProvider>
Expand All @@ -425,11 +423,7 @@ OpenTelemetryPluginImpl::OpenTelemetryPluginImpl(
std::move(generic_method_attribute_filter)),
plugin_options_(std::move(plugin_options)),
tracer_provider_(std::move(tracer_provider)),
tracer_(
tracer_provider_ != nullptr
? tracer_provider_->GetTracer("grpc-c++", GRPC_CPP_VERSION_STRING)
: opentelemetry::nostd::shared_ptr<
opentelemetry::trace::Tracer>()),
tracer_(MaybeMakeTracer(tracer_provider_.get())),
text_map_propagator_(std::move(text_map_propagator)),
channel_scope_filter_(std::move(channel_scope_filter)) {
if (meter_provider_ != nullptr) {
Expand Down Expand Up @@ -683,9 +677,6 @@ OpenTelemetryPluginImpl::OptionalLabelStringToKey(absl::string_view key) {

absl::string_view OpenTelemetryPluginImpl::GetMethodFromPath(
const grpc_core::Slice& path) {
if (path.empty()) {
return "";
}
// Check for leading '/' and trim it if present.
return absl::StripPrefix(path.as_string_view(), "/");
}
Expand Down Expand Up @@ -728,9 +719,7 @@ void OpenTelemetryPluginImpl::AddCounter(
grpc_core::GlobalInstrumentsRegistry::GlobalInstrumentHandle handle,
uint64_t value, absl::Span<const absl::string_view> label_values,
absl::Span<const absl::string_view> optional_values) {
if (meter_provider_ == nullptr) {
return;
}
if (meter_provider_ == nullptr) return;
const auto& instrument_data = instruments_data_.at(handle.index);
if (std::holds_alternative<Disabled>(instrument_data.instrument)) {
// This instrument is disabled.
Expand Down Expand Up @@ -761,9 +750,7 @@ void OpenTelemetryPluginImpl::AddCounter(
grpc_core::GlobalInstrumentsRegistry::GlobalInstrumentHandle handle,
double value, absl::Span<const absl::string_view> label_values,
absl::Span<const absl::string_view> optional_values) {
if (meter_provider_ == nullptr) {
return;
}
if (meter_provider_ == nullptr) return;
const auto& instrument_data = instruments_data_.at(handle.index);
if (std::holds_alternative<Disabled>(instrument_data.instrument)) {
// This instrument is disabled.
Expand Down Expand Up @@ -794,9 +781,7 @@ void OpenTelemetryPluginImpl::RecordHistogram(
grpc_core::GlobalInstrumentsRegistry::GlobalInstrumentHandle handle,
uint64_t value, absl::Span<const absl::string_view> label_values,
absl::Span<const absl::string_view> optional_values) {
if (meter_provider_ == nullptr) {
return;
}
if (meter_provider_ == nullptr) return;
const auto& instrument_data = instruments_data_.at(handle.index);
if (std::holds_alternative<Disabled>(instrument_data.instrument)) {
// This instrument is disabled.
Expand Down Expand Up @@ -829,9 +814,7 @@ void OpenTelemetryPluginImpl::RecordHistogram(
grpc_core::GlobalInstrumentsRegistry::GlobalInstrumentHandle handle,
double value, absl::Span<const absl::string_view> label_values,
absl::Span<const absl::string_view> optional_values) {
if (meter_provider_ == nullptr) {
return;
}
if (meter_provider_ == nullptr) return;
const auto& instrument_data = instruments_data_.at(handle.index);
if (std::holds_alternative<Disabled>(instrument_data.instrument)) {
// This instrument is disabled.
Expand Down Expand Up @@ -862,9 +845,7 @@ void OpenTelemetryPluginImpl::RecordHistogram(

void OpenTelemetryPluginImpl::AddCallback(
grpc_core::RegisteredMetricCallback* callback) {
if (meter_provider_ == nullptr) {
return;
}
if (meter_provider_ == nullptr) return;
std::vector<
std::variant<CallbackGaugeState<int64_t>*, CallbackGaugeState<double>*>>
gauges_that_need_to_add_callback;
Expand Down Expand Up @@ -941,9 +922,7 @@ void OpenTelemetryPluginImpl::AddCallback(

void OpenTelemetryPluginImpl::RemoveCallback(
grpc_core::RegisteredMetricCallback* callback) {
if (meter_provider_ == nullptr) {
return;
}
if (meter_provider_ == nullptr) return;
{
grpc_core::MutexLock lock(&mu_);
callback_timestamps_.erase(callback);
Expand Down Expand Up @@ -1130,17 +1109,16 @@ class GrpcTraceBinTextMapPropagator : public TextMapPropagator {
return;
}
carrier.Set("grpc-trace-bin",
// gRPC C++ does not have RTTI, so we encode bytes to String to
// comply with the TextMapSetter API.
absl::Base64Escape(absl::string_view(
reinterpret_cast<char*>(
SpanContextToGrpcTraceBinHeader(span_context).data()),
kGrpcTraceBinHeaderLen)));
}

bool Fields(opentelemetry::nostd::function_ref<bool(
opentelemetry::nostd::string_view)>) const noexcept override {
return true;
bool Fields(opentelemetry::nostd::function_ref<
bool(opentelemetry::nostd::string_view)>
callback) const noexcept override {
return callback("grpc-trace-bin");
}

private:
Expand Down Expand Up @@ -1199,10 +1177,8 @@ opentelemetry::nostd::string_view GrpcTextMapCarrier::Get(
// Store the string on the arena since we are returning a string_view.
std::string* arena_stored_string =
grpc_core::GetContext<grpc_core::Arena>()->ManagedNew<std::string>(
ret_val);
std::move(ret_val));
return *arena_stored_string;
return AbslStringViewToNoStdStringView(
metadata_->GetStringValue(absl_key, &scratch).value_or(""));
}

void GrpcTextMapCarrier::Set(opentelemetry::nostd::string_view key,
Expand All @@ -1214,8 +1190,9 @@ void GrpcTextMapCarrier::Set(opentelemetry::nostd::string_view key,
if (!absl::Base64Unescape(absl_value, &unescaped_value)) {
return;
}
metadata_->Set(grpc_core::GrpcTraceBinMetadata(),
grpc_core::Slice::FromCopiedString(unescaped_value));
metadata_->Set(
grpc_core::GrpcTraceBinMetadata(),
grpc_core::Slice::FromCopiedString(std::move(unescaped_value)));
} else if (absl::EndsWith(absl_key, "-bin")) {
LOG(ERROR) << "Binary propagator other than GrpcTraceBinPropagator is "
"not supported.";
Expand Down
14 changes: 9 additions & 5 deletions src/cpp/ext/otel/otel_plugin.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -557,11 +557,15 @@ class GrpcTextMapCarrier
grpc_metadata_batch* metadata_;
};

absl::string_view NoStdStringViewToAbslStringView(
opentelemetry::nostd::string_view string);

opentelemetry::nostd::string_view AbslStringViewToNoStdStringView(
absl::string_view string);
inline absl::string_view NoStdStringViewToAbslStringView(
opentelemetry::nostd::string_view string) {
return absl::string_view(string.data(), string.size());
}

inline opentelemetry::nostd::string_view AbslStringViewToNoStdStringView(
absl::string_view string) {
return opentelemetry::nostd::string_view(string.data(), string.size());
}

} // namespace internal
} // namespace grpc
Expand Down
31 changes: 23 additions & 8 deletions test/cpp/ext/otel/grpc_trace_bin_text_map_propagator_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ namespace grpc {
namespace testing {
namespace {

using ::testing::MockFunction;
using ::testing::Return;
using ::testing::StrictMock;

class TestTextMapCarrier
: public opentelemetry::context::propagation::TextMapCarrier {
public:
Expand Down Expand Up @@ -87,15 +91,18 @@ TEST(GrpcTraceBinTextMapPropagatorTest, Inject) {

TEST(GrpcTraceBinTextMapPropagatorTest, Extract) {
TestTextMapCarrier carrier;
std::array<char, 29> trace_bin_val;
memcpy(trace_bin_val.data(),
"\x00\x00"
"0123456789ABCDEF"
"\x01"
"01234567\x02\x01",
29);
constexpr char kTraceBinValue[] =
"\x00" // version
"\x00" // field 0
"0123456789ABCDEF" // trace
"\x01" // field 1
"01234567" // span
"\x02" // field 2
"\x01"; // flag
absl::string_view trace_bin_value(kTraceBinValue, sizeof(kTraceBinValue));
carrier.Set("grpc-trace-bin",
absl::Base64Escape(absl::string_view(trace_bin_val.data(), 29)));
absl::Base64Escape(absl::string_view(
kTraceBinValue, sizeof(kTraceBinValue) - 1)));
auto propagator = experimental::MakeGrpcTraceBinTextMapPropagator();
opentelemetry::context::Context context;
context = propagator->Extract(carrier, context);
Expand All @@ -111,6 +118,14 @@ TEST(GrpcTraceBinTextMapPropagatorTest, Extract) {
EXPECT_EQ(span_context.trace_flags().flags(), 1);
}

TEST(GrpcTraceBinTextMapPropagatorTest, Fields) {
auto propagator = experimental::MakeGrpcTraceBinTextMapPropagator();
StrictMock<MockFunction<bool(opentelemetry::nostd::string_view)>>(
mock_callback);
EXPECT_CALL(mock_callback, Call("grpc-trace-bin")).WillOnce(Return(true));
propagator->Fields(mock_callback.AsStdFunction());
}

} // namespace
} // namespace testing
} // namespace grpc
Expand Down
Loading

0 comments on commit 0860652

Please sign in to comment.