8000 feat: add tracing for sql by f0ssel · Pull Request #1610 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

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.

Sign up for GitHub

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

Merged
merged 17 commits into from
May 20, 2022
Merged

feat: add tracing for sql #1610

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
06ca51b
feat: Move from datadog to generic otel
f0ssel May 18, 2022
f8c3ab7
add telemetry pkg
f0ssel May 18, 2022
93ec818
add telemetry package
f0ssel May 18, 2022
b839175
cleanup data we send
f0ssel May 18, 2022
25d5cb1
add doc comment
f0ssel May 18, 2022
3eeda21
rename to tracing
f0ssel May 18, 2022
69ca227
rename to tracing for real
f0ssel May 18, 2022
af7c5ef
move to coderd
f0ssel May 18, 2022
46d9217
wip
f0ssel May 19, 2022
2005d4c
fix merge
f0ssel May 19, 2022
cc5e0b0
Add span formatter
f0ssel May 19, 2022
5b9b654
lint
f0ssel May 19, 2022
a2ddd77
remove postgres dev hack
f0ssel May 19, 2022
4f401e1
try to fix tests
f0ssel May 20, 2022
2cb35aa
dont use interface
f0ssel May 20, 2022
a4c4683
pr comments
f0ssel May 20, 2022
0f5519b
lint
f0ssel May 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add span formatter
  • Loading branch information
@f0ssel
f0ssel committed May 20, 2022
commit cc5e0b00e401e25ce3af4b63f3df5d91ec718fec
18 changes: 17 additions & 1 deletion coderd/tracing/postgres.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package tracing

import (
"context"
"fmt"
"strings"

"github.com/nhatthm/otelsql"
semconv "go.opentelemetry.io/otel/semconv/v1.4.0"
semconv "go.opentelemetry.io/otel/semconv/v1.7.0"
"go.opentelemetry.io/otel/trace"
"golang.org/x/xerrors"
)
Expand All @@ -17,10 +21,22 @@ func PostgresDriver(tp trace.TracerProvider, service string) (string, error) {
otelsql.TraceQueryWithoutArgs(),
otelsql.WithSystem(semconv.DBSystemPostgreSQL),
otelsql.WithTracerProvider(tp),
otelsql.WithSpanNameFormatter(formatPostgresSpan),
)
if err != nil {
return "", xerrors.Errorf("registering postgres tracing driver: %w", err)
}

return driverName, nil
}

func formatPostgresSpan(ctx context.Context, op string) string {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

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 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?

Copy link
Member

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. 👍🏻

const qPrefix = "-- name: "
q := otelsql.QueryFromContext(ctx)
if q == "" || !strings.HasPrefix(q, qPrefix) {
return strings.ToUpper(op)
}

s := strings.Split(strings.TrimPrefix(q, qPrefix), " ")[0]
Copy link
Member

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.

Suggested change
s := strings.Split(strings.TrimPrefix(q, qPrefix), " ")[0]
s := strings.SplitN(strings.TrimPrefix(q, qPrefix), " ", 1)[0]

Copy link
Contributor Author

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

return fmt.Sprintf("%s %s", strings.ToUpper(op), s)
}
0