-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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.
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
[OTel C++] Add APIs to enable/disable metrics #36183
Conversation
OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::EnableMetric( | ||
absl::string_view metric_name) { | ||
metrics_.emplace(metric_name); | ||
OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::EnableMetrics( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not directly related to this PR, but what's stopping us from eliminating OpenTelemetryPluginBuilderImpl
at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, yeah, really it can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, please remove it as a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't promise ABI compatibility between versions, so that's not something our users can count on anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Yash!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion taken, thanks!
OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::EnableMetric( | ||
absl::string_view metric_name) { | ||
metrics_.emplace(metric_name); | ||
OpenTelemetryPluginBuilderImpl& OpenTelemetryPluginBuilderImpl::EnableMetrics( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, please remove it as a follow-up PR.
No description provided.