feat: Open telemetry implementation #2770

surbhigarg92 · 2024-01-05T09:25:34Z

This PR adds support for OpenTelemetry Instrumentation for Traces and Metrics.

Add dependency for OpenTelemetrySDK and required exporters.

Create OpenTelemetry object with required MeterProvider and TracerProvider exporter . Inject OpenTelemetry object via SpannerOptions or register as Global

`
OpenTelemetry openTelemetry = OpenTelemetrySdk.builder()
.setPropagators(ContextPropagators.create(W3CTraceContextPropagator.getInstance()))
.setTracerProvider(tracerProvider)
.setMeterProvider(sdkMeterProvider)
.build;

SpannerOptions options = SpannerOptions.newBuilder().setOpenTelemetry(openTelemetry).build();
`

By default, OpenTelemetry traces are not enabled. To enable OpenTelemetry traces , call SpannerOptions.enableOpenTelemetryTraces() in startup of your application. Enabling OpenTelemetry traces will disable OpenCensus traces. Both OpenCensus and OpenTelemetry traces can not be enabled at the same time.

olavloite · 2024-01-10T10:12:07Z

README.md

@@ -158,6 +158,9 @@ the Cloud Spanner Java client.

 ## OpenCensus Metrics

+> Note: OpenCensus project is deprecated. See [Sunsetting OpenCensus](https://opentelemetry.io/blog/2023/sunsetting-opencensus/).
+We recommend migrating to OpenTelemetry, the successor project.


Can we add a link here directly to documentation that explains how they should set that up?

I added the complete documentation . PTAL

google-cloud-spanner/pom.xml

google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseClientImpl.java

google-cloud-spanner/src/main/java/com/google/cloud/spanner/IScope.java

google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunnerImpl.java

...-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerInterceptorProvider.java

olavloite · 2024-01-10T10:58:06Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpcViews.java

  public static void registerGfeLatencyAndHeaderMissingCountViews() {
-    viewManager.registerView(SPANNER_GFE_LATENCY_VIEW);
-    viewManager.registerView(SPANNER_GFE_HEADER_MISSING_COUNT_VIEW);
+    if (SpannerOptions.isEnabledOpenCensusMetrics()) {


Here also: What happens if someone switches this bit back and forth for different Spanner instances?

Metrics its little different. OpenCensus Metrics and OpenTelmetry metrics can be enabled or disable separately. So enabling one will not interfere with other. Also customers don't have the option to disable OpenTelemetry metrics .

If customer creates 1 client and enables metrics and creates another spanner client. Customer would only get metrics for the second client which is expected.

Hmm.... I think it will depend on what is called in which order. But as this is OpenCensus, which only supports global configuration, I guess this is the best we can do.

olavloite · 2024-01-10T11:01:18Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/BatchClientImplTest.java

@@ -66,6 +67,7 @@ public final class BatchClientImplTest {
  @Before
  public void setUp() {
    initMocks(this);
+    SpannerOptions.enableOpenTelemetryTraces();


This will enable OpenTelemetry traces for all tests that come after this test. Java does not guarantee the order in which tests are executed. Are we sure that all tests will behave correctly, regardless of the order that they are executed, if this option is enabled at any random point in time?

Where ever we have added OpenTelemetry or OpenCensus related test, there I am resetting this with the new value.

As it is a setting that you want to be enabled globally for all the tests in this class, you should put it in a class setup method. So like this:

@BeforeClass public static void setupOpenTelemetry() { SpannerOptions.enableOpenTelemetryTraces(); }

And put the reset in a cleanup method that is called after all the tests in this class have finished.

@AfterClass public static void resetOpenTelemetry() { SpannerOptions.resetActiveTracingFramework(); }

google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/SpannerRpcMetricsTest.java

olavloite

LGTM, with some nits on the tests. Especially the configuration set/reset strategy should be changed for the tests, so that a test always starts with setting its own desired configuration, and then at the end resets to the default. That will ensure that the configuration for one specific test does not unintentionally leak to another test.

README.md

google-cloud-spanner/src/main/java/com/google/cloud/spanner/OpenCensusSpan.java

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java

olavloite · 2024-01-19T09:28:12Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java

+  public static void disableOpenCensusMetrics() {
+    SpannerOptions.enableOpenCensusMetrics = false;
+  }
+
+  @VisibleForTesting
+  static void enableOpenCensusMetrics() {
+    SpannerOptions.enableOpenCensusMetrics = true;
+  }
+
+  public static boolean isEnabledOpenCensusMetrics() {
+    return SpannerOptions.enableOpenCensusMetrics;
+  }
+
+  /** Enables OpenTelemetry metrics. Enable OpenTelemetry metrics before creating Spanner client. */
+  public static void enableOpenTelemetryMetrics() {
+    SpannerOptions.enableOpenTelemetryMetrics = true;
+  }
+
+  public static boolean isEnabledOpenTelemetryMetrics() {
+    return SpannerOptions.enableOpenTelemetryMetrics;
+  }


We can do it in a separate PR, but I think that these static options should also be guarded by the same lock as the one that we use for activeTracingFramework.

olavloite · 2024-01-19T09:32:58Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpcViews.java

  public static void registerGfeLatencyAndHeaderMissingCountViews() {
-    viewManager.registerView(SPANNER_GFE_LATENCY_VIEW);
-    viewManager.registerView(SPANNER_GFE_HEADER_MISSING_COUNT_VIEW);
+    if (SpannerOptions.isEnabledOpenCensusMetrics()) {


Hmm.... I think it will depend on what is called in which order. But as this is OpenCensus, which only supports global configuration, I guess this is the best we can do.

olavloite · 2024-01-19T09:39:07Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/ResumableStreamIteratorTest.java

+    SpannerOptions.resetActiveTracingFramework();
+    SpannerOptions.enableOpenTelemetryTraces();


Are you sure you need this in a test? If so, then you should do it like this:

try { SpannerOptions.enableOpenTelemetryTraces(); } finally { SpannerOptions.resetActiveTracingFramework() }

olavloite · 2024-01-19T09:40:56Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/ResumableStreamIteratorTest.java

+    SpannerOptions.resetActiveTracingFramework();
+    SpannerOptions.enableOpenCensusTraces();


Here also: We should prefer not to do this kind of set/reset in a separate test, and if we do, then it should preferably be in the other order:

Set the desired configuration

Reset to the default in a finally block

The above will ensure that we don't unintentionally leak a custom configuration to other tests. Now, OpenCensus traces are enabled for any test that follows this test. That can give unintended side effects, as much of this configuration is global.

olavloite · 2024-01-19T09:41:37Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java

+
+    MetricsRecord record = metricRegistry.pollRecord();
+    assertThat(record.getMetrics().size()).isEqualTo(0);
+    SpannerOptions.enableOpenCensusMetrics();


Put in a finally block

olavloite · 2024-01-19T09:42:09Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java

+
+  @Test
+  public void testOpenTelemetrySessionMetrics() throws Exception {
+    SpannerOptions.enableOpenTelemetryMetrics();


Reset this in a finally block (and check other places as well)

olavloite · 2024-01-19T09:43:17Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpanTest.java

+    SpannerOptions.resetActiveTracingFramework();
+    SpannerOptions.enableOpenCensusTraces();


Put the enable... in a @BeforeClass method

Put the reset... in a @afterclass method

This will ensure that the configuration of this test class does not leak into any other tests.

… object

Co-authored-by: Knut Olav Løite <koloite@gmail.com>

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

surbhigarg92 requested review from a team as code owners January 5, 2024 09:25

product-auto-label bot added size: xl Pull request size is extra large. api: spanner Issues related to the googleapis/java-spanner API. labels Jan 5, 2024

surbhigarg92 changed the title ~~Open telemetry implementation~~ feat: Open telemetry implementation Jan 5, 2024

surbhigarg92 force-pushed the OpenTelemetry_Implementation branch from c850e91 to 863dc63 Compare January 10, 2024 09:22

olavloite reviewed Jan 10, 2024

View reviewed changes

surbhigarg92 force-pushed the OpenTelemetry_Implementation branch 19 times, most recently from a2cdecd to f6486a8 Compare January 17, 2024 16:47

surbhigarg92 requested a review from olavloite January 18, 2024 11:02

olavloite approved these changes Jan 19, 2024

View reviewed changes

surbhigarg92 force-pushed the OpenTelemetry_Implementation branch 2 times, most recently from 27626e7 to 5ae1152 Compare February 7, 2024 07:01

surbhigarg92 added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 7, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 7, 2024

surbhigarg92 and others added 14 commits February 8, 2024 12:17

feat: OpenTelemetry Session Metrics and Traces

30aa191

review comments

3907cbc

Removed setSpan method

f0552ba

review comments

aceb177

Metrics enable option

620f445

Bucket boundaries for latency metrics

93b19f6

feat: refactor Spanner metrics and traces to use inject opentelemetry…

bc86c8f

… object

feat: Refactor OpenTelemetry Tracer and Metrics

e1e59e2

Readme

2543160

Review Comments

765edef

test

6e0239e

Update README.md

4a0acfb

Co-authored-by: Knut Olav Løite <koloite@gmail.com>

review comments

b1c5590

🦉 Updates from OwlBot post-processor

a612357

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

surbhigarg92 force-pushed the OpenTelemetry_Implementation branch from e290810 to a612357 Compare February 8, 2024 06:53

surbhigarg92 added automerge Merge the pull request once unit tests and other checks pass. owlbot:run Add this label to trigger the Owlbot post processor. labels Feb 8, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 8, 2024

gcf-merge-on-green bot merged commit 244d6a8 into googleapis:main Feb 8, 2024
24 checks passed

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 8, 2024

release-please bot mentioned this pull request Feb 8, 2024

chore(main): release 6.58.0 #2859

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Open telemetry implementation #2770

feat: Open telemetry implementation #2770

surbhigarg92 commented Jan 5, 2024

olavloite Jan 10, 2024

surbhigarg92 Jan 10, 2024

olavloite Jan 10, 2024

surbhigarg92 Jan 17, 2024

olavloite Jan 19, 2024

olavloite Jan 10, 2024

surbhigarg92 Jan 17, 2024

olavloite Jan 19, 2024

olavloite left a comment

olavloite Jan 19, 2024

olavloite Jan 19, 2024

olavloite Jan 19, 2024

olavloite Jan 19, 2024

olavloite Jan 19, 2024

olavloite Jan 19, 2024

olavloite Jan 19, 2024

		SpannerOptions.resetActiveTracingFramework();
		SpannerOptions.enableOpenTelemetryTraces();

		SpannerOptions.resetActiveTracingFramework();
		SpannerOptions.enableOpenCensusTraces();

feat: Open telemetry implementation #2770

feat: Open telemetry implementation #2770

Conversation

surbhigarg92 commented Jan 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olavloite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment