feat: Introduce interfaces for metrics instrumentation #2403
Conversation
product-auto-label
bot
added
size: l
blakeli0
changed the title
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallContext.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/BaseApiTracer.java
Show resolved
Hide resolved
| * Creates a new instance of the RPC method name. | ||
| * | ||
| * @param serviceName The name of the service. In general this will be GAPIC generated service | ||
| * name {@link StubSettings#getServiceName()}. However, in some cases, when the GAPIC |
There was a problem hiding this comment.
@blakeli0 I am now having some second thoughts on the serviceName() getter's name. I think there might be some slight discrepancy between the intended result and the function name.
getServiceName() returns the name that was configured in the default_host. For example, asset's will return cloudasset: https://github.com/googleapis/google-cloud-java/blob/9785f8cfd43db0a8968f086a1461242da9d16cb5/java-asset/google-cloud-asset/src/main/java/com/google/cloud/asset/v1/stub/AssetServiceStubSettings.java#L866-L868
even though the intended result for this probably should be asset.
Not something blocking this PR, but just wondering your thoughts on this.
There was a problem hiding this comment.
@lqiu96 why should the intended name be
asset? @blakeli0 Is there any way we could test this? that for a particular service, this is the intented serviceName ?
I don't want to say what the intended name should be. I think it might be a bit weird for customers since we probably aren't providing consistent service names.
java-asset's serviceName is cloudasset (link above)
java-biglake's serviceName is biglake (https://github.com/googleapis/google-cloud-java/blob/458516b462e0a4494f32815fe9d0e6f0b30353f1/java-biglake/google-cloud-biglake/src/main/java/com/google/cloud/bigquery/biglake/v1/stub/MetastoreServiceStubSettings.java#L409-L411)
I think most services would have the service name of java-{serviceName} except for asset and possibly a few others.
It might not be a big concern after all, but something just doesn't sit right with me knowing that serviceName is parsed from a URI (default_host value) and not the package name or some config value (name_pretty or api name).
There was a problem hiding this comment.
I spent sometime while working on the dashboard on service names, and they are not of the type "java-{serviceName}". See column A in this sheet. In the dashboard too, we are deriving the service name from the default_host value.
There was a problem hiding this comment.
FYI, your link doesn't work for me. Seems like there is no defined serviceName and we've somehow settled on the same definition 😆
Not blocking this PR, but it would be great if there was an official definition for serviceName. Perhaps a question for the core team.
There was a problem hiding this comment.
@lqiu96 I think the term serviceName you introduced still makes sense, because we call the yaml and json config for each versioned protos service yaml and service config respectively. we may want to add more docs to explain that it is used for constructing service endpoints.
@ddixit14 I would call java-{serviceName} a library name or repo name, as it could includes multiple versioned services or admin services.
There was a problem hiding this comment.
Ok, to wrap up the conversation above: I'll add some additional docs clarifying the intended use case (constructing the endpoint) and how the this value is constructed (from the config files).
It is fine that serviceName in the library (pulled from getServiceName()) doesn't 100% match java-{serviceName}.
There was a problem hiding this comment.
It is fine that serviceName in the library (pulled from
getServiceName()) doesn't 100% matchjava-{serviceName}.
Correct. The library/repo name may not always be in the format of java-{serviceName} and I would not refer java-{serviceName} as the format for library/repo name.
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsRecorder.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsRecorder.java
Outdated
Show resolved
Hide resolved
|
|
This PR introduces new public interfaces and refactors existing interfaces for metrics instrumentation. All the interfaces are framework-agnostic. Subsequent PRs should implement interfaces with an observability framework. For example, introduce an
OpenTelemetryMetricsRecorderthat implementsMetricsRecorder. All new methods are created with Javadocs, they are expected to be updated in the implementation PR if needed.See go/java-gapic-otel-metrics-design for details.