feat: add tracing for sql #1610
Conversation
|
|
||
| sqlDriver, err = tracing.PostgresDriver(tracerProvider, "coderd.database") | ||
| if err != nil { | ||
| logger.Warn(cmd.Context(), "failed to start postgres tracing driver", slog.Error(err)) |
There was a problem hiding this comment.
potential bug: If tracing.PostgresDriver encounters an error here, sqlDriver will be the empty string; I believe that would cause us to fail to connect to the DB. We should probably fall back to the original driver name in this case and degrade gracefully.
| logger.Warn(cmd.Context(), "failed to start postgres tracing driver", slog.Error(err)) | |
| logger.Warn(cmd.Context(), "failed to start postgres tracing driver", slog.Error(err)) | |
| sqlDriver = "postgres" |
| span.SetAttributes(semconv.NetAttributesFromHTTPRequest("tcp", r)...) | ||
| span.SetAttributes(semconv.EndUserAttributesFromHTTPRequest(r)...) | ||
| span.SetAttributes(semconv.HTTPServerAttributesFromHTTPRequest("", route, r)...) | ||
| span.SetAttributes(semconv.HTTPRouteKey.String(route)) |
There was a problem hiding this comment.
Nice change with the semconv package!
| return driverName, nil | ||
| } | ||
|
|
||
| func formatPostgresSpan(ctx context.Context, op string) string { |
There was a problem hiding this comment.
Should we add the sql: prefix (or coder-db:), like in https://pkg.go.dev/github.com/nhatthm/otelsql#readme-span-name-formatter? Wondering if it'd help with narrowing down the spans.
There was a problem hiding this comment.
I'm just copying the format I've seen before with other database tracing tools (New Relic) and also mimicing the format of the http spans. Since an http span is METHOD route, I followed a similar format of OP name so the spans look like QUERY GetAllUsersById.
I don't love the prefix because I already can see where the span is coming from by the instrumentation library (see the screenshots in description), but I also don't know if this is the case on other tools like Jaeger, which I'd like to setup eventually. I'm down to change it if we find it hard to parse as we use it, does that sound ok?
There was a problem hiding this comment.
Yeah, that sounds perfectly reasonable. 👍🏻
coderd/tracing/postgres.go
Outdated
| return strings.ToUpper(op) | ||
| } | ||
|
|
||
| s := strings.Split(strings.TrimPrefix(q, qPrefix), " ")[0] |
There was a problem hiding this comment.
Just a small perf suggestion to avoid processing the entire query.
| s := strings.Split(strings.TrimPrefix(q, qPrefix), " ")[0] | |
| s := strings.SplitN(strings.TrimPrefix(q, qPrefix), " ", 1)[0] |
There was a problem hiding this comment.
of course! I was only paying attention to the first line lol
This adds tracing for the postgres data layer. It also continues to improve on the http tracing by using a lot more built-in functions from
semconv.Datadog example:
SQL -
HTTP -
