-
Notifications
You must be signed in to change notification settings - Fork 33
WIP: tracing #41
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
Draft
flowchartsman
wants to merge
5
commits into
jackc:master
Choose a base branch
from
flowchartsman:tracing
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
WIP: tracing #41
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Might need something similar for tracing resource lifecycle stuff, since there's no context integration into Release() et al.
Some selected
|
flowchartsman
commented
Apr 26, 2025
flowchartsman
commented
Apr 26, 2025
flowchartsman
commented
Apr 26, 2025
flowchartsman
commented
Apr 26, 2025
this change reduces the need to have two separate calls fo trace.acquireEnd for each instance (warm vs. new resource)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Was reading #30 and thought the tracing idea sounded interesting, so I thought I'd do a quick WIP of the idea to play around with the technique and maybe use it in my own projects. Interested in feedback.
Still fiddling with the ergonomics, but I like the abstraction of
tracerSpan
combined with defer, since it helps it get out of the way. Also not too sure what should go inAcquireStartData
beyond the start time. It might be more useful only to trackAcquireEnd
. Maybe @jackc cares to weigh in, since you've had three years of experience with the technique now.Ended up going with the big interface, but that's hardly solid, though splitting it up would require picking a required subset, much as pgx does with
QueryTracer
, and I'm not sure what subset that should be. TheBaseTracer
type also allows the caller to implement any subset of tracer methods and that might need adjustment if you want to ensure that implementations support both methods for each tracing type, but these are basic implementation details and easy enough to change with more granular interfaces and a new span type for each one.Probably also want to add some benchmarking.
Happy to keep going if there's interest.