[OTel] Experimental API for metrics #35348
Conversation
yashykt
added
the
release notes: yes
grpc-checks
bot
added
bloat/none
per-call-memory/increase
per-channel-memory/neutral
labels
src/cpp/ext/otel/otel_plugin.cc
Outdated
| } // namespace internal | ||
|
|
||
| namespace experimental { | ||
| absl::string_view OpenTelemetryClientAttemptStartedInstrumentName() { |
There was a problem hiding this comment.
We may have already discussed this, but is there a reason why these need to be functions instead of just constexpr constants?
There was a problem hiding this comment.
I had tried that some years ago but had ran into issues. Let me try it again and see if it works now.
There was a problem hiding this comment.
pushed a canary change to see if the tests are happy with it. If they are, I'll use it for all.
There was a problem hiding this comment.
seems to be working. Replaced all
| namespace grpc { | ||
|
|
||
| namespace internal { | ||
| class OpenTelemetryPluginBuilderImpl; |
There was a problem hiding this comment.
I assume that once we have the plugin option API in place, there will no longer be any need for having two different layers here (the private API wrapped inside of the public API)?
There was a problem hiding this comment.
Yeah, I think once the experimental CsmObservability API goes away, we won't need to do this.
There was a problem hiding this comment.
It seems like we need the CSM plugin option implemented before we can remove the CsmObservability API. And once we have the CSM plugin option implemented, the CsmObservability code can use that plugin option in the public API, so it won't need to reach inside the internal API anymore. Or am I missing something here?
There was a problem hiding this comment.
I see what you mean. I wasn't originally planning on changing the implementation of CsmObservability to use the plugin option, but I like that idea. I will be able to remove the internal API earlier. Will do that. Thanks!
grpc-checks
bot
added
per-call-memory/neutral
per-call-memory/increase
and removed
per-call-memory/increase
per-call-memory/neutral
labels
grpc-checks
bot
added
per-call-memory/neutral
and removed
per-call-memory/increase
labels
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
Just a couple of lingering comments -- feel free to address or ignore.
include/grpcpp/ext/otel_plugin.h
Outdated
| /// grpc.server.call.started | ||
| /// grpc.server.call.duration | ||
| /// grpc.server.call.sent_total_compressed_message_size | ||
| /// grpc.server.call.rcvd_total_compressed_message_size |
There was a problem hiding this comment.
Probably no need to list the metrics in the comment now that the constants are in the header file directly.
| absl::string_view OpenTelemetryTargetKey() { return "grpc.target"; } | ||
|
|
||
| namespace { | ||
| absl::flat_hash_set<std::string> BaseMetrics() { |
There was a problem hiding this comment.
Does this need to be std::string instead of absl::string_view?
There was a problem hiding this comment.
I want to intentionally store the set as std::string instead of absl::string_view. The case I'm worried about is when in the future we allow users to provide the set of metric names that they want enabled and they provide non-constant strings
grpc-checks
bot
added
per-call-memory/increase
and removed
per-call-memory/neutral
labels
2 participants
Provide a public experimental API and bazel compatible build target for OpenTelemetry metrics.
Details -
OpenTelemetryPluginBuilderclass that provides the API specified in https://github.com/grpc/proposal/blob/master/A66-otel-stats.mdgrpc::internal::OpenTelemetryPluginBuilderclass is moved togrpc::internal::OpenTelemetryPluginBuilderImplfor disambiguation.OTelin some instances toOpenTelemetryfor consistency.