googleapis / googleapis/google-cloud-go

spanner: panic in recordGFELatencyMetricsOT if EnableOpenTelemetryMetrics called after NewClient

Open
#9,519 1 comment 1 reaction 1 assignee Claimed by @rahul2393 View on GitHub
api: spanner priority: p2 type: question
Dominant language
Go
Stars
4.5k
Forks
1.6k
Avg merge
1d 13h
Merged PRs (30d)
109

Description

**Client**

Spanner

**Environment**

e.g. Alpine Docker on GKE

**Go Environment**

go1.22.0

**Code**

When enabling open telemetry after creating the client the configuration set on the client is invalid resulting in potential panic.

e.g.
```go
package main

func main() {
// ...
client, err := gcspanner.NewClient(
ctx,
db,
opts...,
)

if err != nil {
panic(err)
}

initOnce.Do(func() {
if os.Getenv("SPANNER_EMULATOR_HOST") == "" {
spanner.EnableOpenTelemetryMetrics()
}
})

// ...
}
```

In the above example the new client created would have a default `openTelemetryConfig` with all instruments being nil possibly leading to a panic during a call to `recordGFELatencyMetricsOT` (see bellow with added comments)

```go

func recordGFELatencyMetricsOT(ctx context.Context, md metadata.MD, keyMethod string, otConfig *openTelemetryConfig) error {
if !IsOpenTelemetryMetricsEnabled() || md == nil && otConfig == nil { // the global flag is enable thus we don't return nil
return nil
}
attr := otConfig.attributeMap
if len(md.Get("server-timing")) == 0 && otConfig.gfeHeaderMissingCount != nil { // at this point otConfig.gfeHeaderMissingCount is nil
otConfig.gfeHeaderMissingCount.Add(ctx, 1, metric.WithAttributes(attr...))
return nil
}
serverTiming := md.Get("server-timing")[0] // This will panic !
gfeLatency, err := strconv.Atoi(strings.TrimPrefix(serverTiming, "gfet4t7; dur="))
if !strings.HasPrefix(serverTiming, "gfet4t7; dur=") || err != nil {
return err
}
attr = append(attr, attributeKeyMethod.String(keyMethod))
if otConfig.gfeLatency != nil {
otConfig.gfeLatency.Record(ctx, int64(gfeLatency), metric.WithAttributes(attr...))
}
return nil
}

```

**Expected behavior**

- The EnableOpenTelemetryMetrics function should mention that it needs to be registered prior to enabling the client
- recordGFELatencyMetricsOT should have appropriate checks to handle reading from metadata while avoiding panics

**Actual behavior**

- EnableOpenTelemetryMetrics doens't mention that it should be called prior to instantiating a new client
- recordGFELatencyMetricsOT panics in some specific conditions

**Additional context**

e.g. Started after upgrading to v1.57.0 and using the `EnableOpenTelemetryMetrics` function as shown above

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.