-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(spanner): add OpenTelemetry implementation #9254
feat(spanner): add OpenTelemetry implementation #9254
Conversation
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 |
There was a problem hiding this comment.
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:
err != nil
: return the error!strings.HasPrefix(serverTiming, "gfet4t7; dur=")
: This should be considered the same as a missing GFE header (or malformed GFE header). Now we just returnnil
when that happens, is that intentional?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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")))
const metricsPrefix = "spanner/" | ||
|
||
var ( | ||
attributeKeyClientID = attribute.Key("client_id") |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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")))
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
spanner.EnableOpenTelemetryMetrics()
in startup of your application.Tracing
GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING
toopentelemetry