chore: client metrics

[surbhigarg92]

This PR adds built-in metrics to the Java client library. This will introduce below metrics for the consumers of this library .

This feature is not currently available for customers to use. Once GA'ed consumers of this library will start getting these metrics in the MetricsExplorer

Attempt Latency	The latency of an RPC attempt.	Useful for monitoring the general health of the applicationWith attributes like “Token Expiry”, “Status” etc, this metric can help us troubleshoot a wide range of issues.
Attempt Count	The count of RPC attempts	Useful for monitoring trafficUseful for deriving “Error Rate” with status attributes
Operation Latency	The total end-to-end latency across all request attempts, including retries.	Useful for monitoring the general health of the applicationThe combination of this metric and Attempt Latency can help us troubleshoot connectivity issues.
Operation Count	The count of operations	Useful for monitoring trafficUseful for deriving “Error Rate” with status labelThe combination of this metric and Attempt Count can be used for deriving Retry Count

[conventional-commit-lint-gcf]

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

[olavloite]

If it is only for testing, can we then use the following trick to make this package-private:

1. Make this package-private
2. Add a public static method to SpannerOptionsTestHelper that takes a SpannerOptions.Builder instance as an argument, and that calls this setEnableBuiltInMetrics(...) method.
3. Call the SpannerOptionsTestHelper method from the tests that need to set this value.

[olavloite]

I think that this should be:

Suggested change

	boolean isDirectPathChannelCreated =
	defaultChannelProvider.canUseDirectPath()
	&& defaultChannelProvider.isDirectPathXdsEnabled();
	boolean isDirectPathChannelCreated =
	options.getChannelProvider() == null && defaultChannelProvider.canUseDirectPath()
	&& defaultChannelProvider.isDirectPathXdsEnabled();

[olavloite]

A get-method should preferably not call something that creates a new instance every time it is called. The original implementation of this did create a new list every time it was called, but it did not create any of the elements in the list. Can we make sure that this method still does not create a new instance of ApiTracerFactory each time it is called?

[surbhigarg92]

Yes, even the metricstracerfactory getting added is only created once. We are using the static instance of BuiltInOpenTelemetryMetricsProvider to call getOrCreateOpenTelemetry . We have a check in this if openTelemetry is already created, then we do not create it again.

It will create a new MetricsTracerFactory object with the same openTelemetry object similar to OpenTelemetryApiTracerFactory

[olavloite]

Strictly speaking, we shouldn't add these attributes to the compositeTracer, but only to the underlying built-in metrics tracer. Now the attributes will also be added to any other MetricsTracer that is part of the composite tracer, but it only happens if you have built-in tracing enabled.
Meaning that:

1. Enabling built-in metrics will obviously enable built-in metrics.
2. But it will also have the side-effect that any other metrics-tracer that you had enabled will suddenly get two additional attributes.

Is there any way that we can avoid that side-effect?

[surbhigarg92]

I didn't quite get you. We have a check in the CompositeTracer to pass these attributes further only for MetricsTracer. We only have one MetricsTracer

public void addAttributes(String key, String value) {
    for (ApiTracer child : children) {
      if (child instanceof MetricsTracer) {
        MetricsTracer metricsTracer = (MetricsTracer) child;
        metricsTracer.addAttributes(key, value);
      }
    }
  };

[olavloite]

Cannot a user create and add a MetricsTracer themselves?

[surbhigarg92]

MetricsTracer class is decorated with @internalapi annotation . So we don't recommend customers to directly use this class. But even if they use this class to register metricstracerfactory, then yes they would get extra attributes.

[olavloite]

Can we add a more descriptive pull request title than client metrics, and also add a description to the pull request? That will improve the release notes that are generated from this change.

If it is something that is not yet GA, but still needs to be merged now, then we should rename the pull request title to chore:....

[surbhigarg92]

If it is only for testing, can we then use the following trick to make this package-private: Make this package-private
Add a public static method to SpannerOptionsTestHelper that takes a SpannerOptions.Builder instance as an argument, and that calls this setEnableBuiltInMetrics(...) method.
Call the SpannerOptionsTestHelper method from the tests that need to set this value.

I have actually removed public from setEnableBuiltInMetrics . It was only required for ITBuiltinMetricsTest which we are not enabling right now anyway. Lets not give an option to customers to enable it till the time we actually GA our feature.