feat: add OpenTelemetry tracing #1182

olavloite · 2023-11-12T20:21:08Z

Adds OpenTelemetry tracing and configures Google Cloud Trace exports.

Adds OpenTelemetry tracing and metrics, and configures Google Cloud Trace and Google Cloud Monitoring exports.

codecov · 2023-11-12T20:46:10Z

Codecov Report

Attention: 89 lines in your changes are missing coverage. Please review.

Comparison is base (397f1a2) 90.53% compared to head (7aa87f2) 89.95%.
Report is 4 commits behind head on postgresql-dialect.

Files	Patch %	Lines
...ava/com/google/cloud/spanner/pgadapter/Server.java	0.00%	49 Missing ⚠️
...ud/spanner/pgadapter/metadata/OptionsMetadata.java	21.05%	27 Missing and 3 partials ⚠️
...panner/pgadapter/statements/BackendConnection.java	92.68%	6 Missing and 3 partials ⚠️
...ner/pgadapter/statements/SimpleQueryStatement.java	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##             postgresql-dialect    #1182      +/-   ##
========================================================
- Coverage                 90.53%   89.95%   -0.58%     
- Complexity                 2561     2587      +26     
========================================================
  Files                       141      141              
  Lines                      8652     8874     +222     
  Branches                   1263     1285      +22     
========================================================
+ Hits                       7833     7983     +150     
- Misses                      557      622      +65     
- Partials                    262      269       +7

Flag	Coverage Δ
all_tests	`89.95% <72.18%> (-0.58%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

KiranmayiB

Can you please link me to the guide you used for Otel integration?
It looks very different from https://opentelemetry.io/docs/instrumentation/java/manual/

KiranmayiB · 2023-11-16T05:42:52Z

docs/open_telemetry.md

+
+In addition, PGAdapter always adds an exporter for Google Cloud Trace.
+
+## Traces


Can you please add examples for metrics also here?

We don't add any metrics in this pull request, only traces.

Alright sounds good then. Thanks!

KiranmayiB · 2023-11-16T06:14:19Z

src/main/java/com/google/cloud/spanner/pgadapter/Server.java

      server.startServer();
    } catch (Exception e) {
      printError(e, System.err, System.out);
    }
  }

+  static OpenTelemetry setupOpenTelemetry(OptionsMetadata optionsMetadata) throws IOException {
+    if (!optionsMetadata.isEnableOpenTelemetry()) {
+      return OpenTelemetry.noop();


Why is this noop?

Returning a noop OpenTelemetry instance instead of null when OpenTelemetry has not been enabled makes the rest of the code simpler, as we don't need null checks everywhere.

KiranmayiB · 2023-11-16T06:18:03Z

src/main/java/com/google/cloud/spanner/pgadapter/metadata/OptionsMetadata.java

@@ -218,6 +222,20 @@ public Builder setRequireAuthentication() {
      return this;
    }

+    /** Enables OpenTelemetry tracing for PGAdapter. */
+    public Builder setEnableOpenTelemetry() {


We are planning to make the Otel metrics in the client library as opt-in by default. So i think we should make this also as opt-out.

What exactly do you mean by opt-in by default. Does that mean that you will enable it by default? If so, what exactly do you mean with enabled by default?

Does it mean 'we enable OpenTelemetry by default, but we don't by default add any exporters'? If so, I would argue that this is still the same as the feature being an opt-in, as you still need to add an exporter before you actually collect any metrics.

Or does it mean that we enable both OpenTelemetry and add an exporter (e.g. Google Cloud Tracer) by default? That would mean that we by default enable a feature that could incur additional costs for the customer. Is that intentional?

I meant the latter. But I realize now that itsn not possible for traces as we cannot specify a namespace. So lets leave this as-is. Thank you!

olavloite · 2023-11-16T08:58:29Z

https://opentelemetry.io/docs/instrumentation/java/manual/

This setup uses the OpenTelemetry automatic configuration module: https://opentelemetry.io/docs/instrumentation/java/manual/#automatic-configuration

The reason for this is that PGAdapter is mostly used as a standalone application, and not as a library. That means that configuration must happen on the command line when starting the application, instead of programmatic configuration that would be logical in a library (e.g. the Java client). The reasoning here is:

We need command line configuration, as PGAdapter is mostly used as a standalone application, and not as an in-process dependency (which by the way is also possible, but less frequently used).
Instead of re-inventing the wheel and adding a lot of command line arguments to PGAdapter, we use the library offered by OpenTelemetry to do command line configuration (in this case through Java System properties and/or Environment variables).
We override the defaults of OpenTelemetry if the user has not specified any exporters. The default in the automatic configuration module of OpenTelemetry is to use the oltp exporter (see https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/README.md#exporters). That requires an OpenTelemetry endpoint, which we by default do not have. Instead, we set the default exporter that is used by the auto-configuration module to none, but allow the user to specify one using a system property or environment variable.
In addition, we by default add a Google Trace exporter if OpenTelemetry has been enabled.

KiranmayiB

Otel integration LGTM. Thanks a lot!

feat: add OpenTelementry tracing and metrics

6f4e93f

Adds OpenTelemetry tracing and metrics, and configures Google Cloud Trace and Google Cloud Monitoring exports.

olavloite added 2 commits November 13, 2023 13:06

feat: additional attributes for better tracking

2e4899c

docs: add documentation about OpenTelemetry

6cae88f

olavloite marked this pull request as ready for review November 14, 2023 08:29

olavloite requested a review from pratickchokhani November 14, 2023 08:29

KiranmayiB reviewed Nov 16, 2023

View reviewed changes

KiranmayiB approved these changes Nov 16, 2023

View reviewed changes

olavloite mentioned this pull request Nov 20, 2023

feat: emit warning when read/write transaction is retried #1192

Open

olavloite added 5 commits November 21, 2023 17:41

feat: start span at first byte + record metadata parsing

a4b2cc0

feat: add spans for regex matching and sending results

bb2c1b8

fix: use only one tracer

7465467

chore: increase timeout and decrease default sample rate

2305524

docs: document default sampling rate

7aa87f2

olavloite changed the title ~~feat: add OpenTelementry tracing and metrics~~ feat: add OpenTelemetry tracing Nov 22, 2023

olavloite merged commit 26217a3 into postgresql-dialect Nov 22, 2023
30 of 32 checks passed

olavloite deleted the open-telemetry branch November 22, 2023 16:37

release-please bot mentioned this pull request Nov 22, 2023

chore(postgresql-dialect): release 0.27.0 #1179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add OpenTelemetry tracing #1182

feat: add OpenTelemetry tracing #1182

olavloite commented Nov 12, 2023 •

edited

Loading

codecov bot commented Nov 12, 2023 •

edited

Loading

KiranmayiB left a comment

KiranmayiB Nov 16, 2023

olavloite Nov 16, 2023

KiranmayiB Nov 16, 2023

KiranmayiB Nov 16, 2023

olavloite Nov 16, 2023

KiranmayiB Nov 16, 2023

KiranmayiB Nov 16, 2023

olavloite Nov 16, 2023

KiranmayiB Nov 16, 2023

olavloite commented Nov 16, 2023

KiranmayiB left a comment


		In addition, PGAdapter always adds an exporter for Google Cloud Trace.

		## Traces

feat: add OpenTelemetry tracing #1182

feat: add OpenTelemetry tracing #1182

Conversation

olavloite commented Nov 12, 2023 • edited Loading

codecov bot commented Nov 12, 2023 • edited Loading

Codecov Report

KiranmayiB 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olavloite commented Nov 16, 2023

KiranmayiB left a comment

Choose a reason for hiding this comment

olavloite commented Nov 12, 2023 •

edited

Loading

codecov bot commented Nov 12, 2023 •

edited

Loading