Skip to content
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

add context to custom trace name function to allow passing context va… #37

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

realrunner
Copy link

…lues to name queries

@obitech
Copy link
Member

obitech commented Jul 16, 2024

Thanks for the PR! On first glance, hiding the sql_name in the context seems to be a bit a "magic" to me and this feature might not be immediately clear to every user. Could you give us a little more context (hehe) and/or an example how you intend to use this feature?

@costela
Copy link
Member

costela commented Jul 22, 2024

I think sql_name is just a test placeholder. AFAICT the idea is to extract the span name from more than just the statement. E.g.: you may have the same statement being called in multiple parts of the code, each with a different context. @realrunner is that assumption correct?

I kinda see the use-case, but I'm a bit skeptical that it's a good trade-off: the big advantage of traces compared to e.g. logs is their hierarchical nature. I'd expect the parent spans to give you more than enough information without having to cram more stuff in the span's name?

"strings"
"testing"
)

type ctxKey string
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's use zero sized structs instead, to save allocations

Suggested change
type ctxKey string
type ctxKeySQLName struct{}

(even if this is just a test, it's probably a good idea to keep to good practices)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants