Skip to content

feat(spanner): add OpenTelemetry implementation#9254

Merged
harshachinta merged 53 commits intogoogleapis:mainfrom
harshachinta:open-telemetry-instrumentation
Feb 8, 2024
Merged

feat(spanner): add OpenTelemetry implementation#9254
harshachinta merged 53 commits intogoogleapis:mainfrom
harshachinta:open-telemetry-instrumentation

Conversation

@harshachinta
Copy link
Copy Markdown
Contributor

@harshachinta harshachinta commented Jan 12, 2024

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

Customer applications should add dependency for Metrics SDK, Traces SDK and required exporters.

Metrics

  1. By default, OpenTelemetry metrics are not enabled. To enable call spanner.EnableOpenTelemetryMetrics() in startup of your application.
  2. Create MeterProvider and inject it via ClientConfig or register as Global.
client, err := spanner.NewClientWithConfig(ctx, <database-id>, spanner.ClientConfig{
	OpenTelemetryMeterProvider: meterProvider,
})

Tracing

  1. By default, OpenTelemetry traces are not enabled. To enable set environment variable GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING to opentelemetry
  2. Create TracerProvider and register as Global

@harshachinta harshachinta requested review from a team January 12, 2024 16:43
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the Spanner API. labels Jan 12, 2024
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Jan 13, 2024
serverTiming := md.Get("server-timing")[0]
gfeLatency, err := strconv.Atoi(strings.TrimPrefix(serverTiming, "gfet4t7; dur="))
if !strings.HasPrefix(serverTiming, "gfet4t7; dur=") || err != nil {
return err
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit of a theoretical case, but shouldn't we split this into two separate things:

  1. err != nil: return the error
  2. !strings.HasPrefix(serverTiming, "gfet4t7; dur="): This should be considered the same as a missing GFE header (or malformed GFE header). Now we just return nil when that happens, is that intentional?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of malformed GFE header we return an error, not nil.
We are not counting malformed GFE as missing GFE because in this case we get a GFE header but it is not in required format.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

note that this could still return err=nil, if the actual text is only a number without any other prefixes or suffixes. The reason is that strings.TrimPrefix returns the original string if the string did not have the prefix.

This for example prints 4:

fmt.Println(strconv.Atoi(strings.TrimPrefix("4", "World")))

@harshachinta harshachinta added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 20, 2024
const metricsPrefix = "spanner/"

var (
attributeKeyClientID = attribute.Key("client_id")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could the variables here that never change be const?

serverTiming := md.Get("server-timing")[0]
gfeLatency, err := strconv.Atoi(strings.TrimPrefix(serverTiming, "gfet4t7; dur="))
if !strings.HasPrefix(serverTiming, "gfet4t7; dur=") || err != nil {
return err
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

note that this could still return err=nil, if the actual text is only a number without any other prefixes or suffixes. The reason is that strings.TrimPrefix returns the original string if the string did not have the prefix.

This for example prints 4:

fmt.Println(strconv.Atoi(strings.TrimPrefix("4", "World")))

@harshachinta harshachinta removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 8, 2024
@harshachinta harshachinta merged commit fc51cc2 into googleapis:main Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API. size: xl Pull request size is extra large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants