- Notifications
You must be signed in to change notification settings - Fork928
feat: add tracing for sql#1610
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
potential bug: Iftracing.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" |
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.
Very clean, and good catch with reassigning the request context. LGTM!
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice change with thesemconv package!
| return driverName, nil | ||
| } | ||
| func formatPostgresSpan(ctx context.Context, op string) string { |
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.
Should we add thesql: prefix (orcoder-db:), like inhttps://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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 isMETHOD route, I followed a similar format ofOP name so the spans look likeQUERY 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
of course! I was only paying attention to the first line lol
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.
This looks great! I just have one concern regarding the case where we fail to register the tracing driver.
Uh oh!
There was an error while loading.Please reload this page.
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 -
