[OTel C++] Add APIs to enable/disable metrics #36183
Conversation
grpc-checks
bot
added
per-call-memory/decrease
per-channel-memory/neutral
bloat/none
labels
yashykt
added
the
release notes: yes
yashykt
changed the title
| OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::EnableMetric( | ||
| absl::string_view metric_name) { | ||
| metrics_.emplace(metric_name); | ||
| OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::EnableMetrics( |
There was a problem hiding this comment.
Not directly related to this PR, but what's stopping us from eliminating OpenTelemetryPluginBuilderImpl at this point?
There was a problem hiding this comment.
Currently, OpenTelemetryPluginBuilderImpl has the following additional methods -
SetTargetSelector- This will go away after my next PR to add channel scope filter.SetServerSelector- This is unused at the moment and can be removed.TestOnlyEnabledMetrics- Used for testing to see which metrics are enabled.
There was a problem hiding this comment.
so, yeah, really it can be removed
There was a problem hiding this comment.
Okay, please remove it as a follow-up PR.
There was a problem hiding this comment.
Isn't there some benefits with the Pimpl idiom used here? e.g. if we change the implementation details of the OpenTelemetryPluginBuilderImpl, the users of the OpenTelemetryPluginBuilder API do not need to recompile their code (as long as the API does not change).
There was a problem hiding this comment.
We don't promise ABI compatibility between versions, so that's not something our users can count on anyway.
include/grpcpp/ext/otel_plugin.h
Outdated
| // Methods to manipulate which instruments are enabled in the OpenTelemetry | ||
| // Stats Plugin. | ||
| OpenTelemetryPluginBuilder& EnableMetrics( | ||
| const std::vector<absl::string_view>& metric_names); |
There was a problem hiding this comment.
absl::Span<> might be another choice? It would allow the API to take more types of input (e.g. C-style array, initializer list) without constructing a vector.
OpenTelemetryPluginBuilder& EnableMetrics(
absl::Span<const absl::string_view> metric_names);
There was a problem hiding this comment.
Suggestion taken, thanks!
grpc-checks
bot
added
per-call-memory/neutral
and removed
per-call-memory/decrease
labels
| OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::EnableMetric( | ||
| absl::string_view metric_name) { | ||
| metrics_.emplace(metric_name); | ||
| OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::EnableMetrics( |
There was a problem hiding this comment.
Okay, please remove it as a follow-up PR.
No description provided.