-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
2020 05 tracing bucket #2676
2020 05 tracing bucket #2676
Conversation
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
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
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.
Uhm... has been merged while I was reviewing. I find this very useful, but I'm also a bit worried about spans flooding (like we've already seen for memcached, which needs to be fixed... I will submit a PR today). I've also left a couple of comments I would like you to take a look.
} | ||
|
||
func (t TracingBucket) Name() string { | ||
return "tracing: " + t.bkt.Name() |
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.
Not sure if we should rename this. Why not just returning t.bkt.Name()
? We're just adding instrumentation, without any logic change.
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 method is only called by metrics bucket, which is not used on top of tracking bucket. Personally I would suggest removing this method completely.
t.read += n | ||
} | ||
if err != nil && err != io.EOF && t.s != nil { | ||
t.s.LogKV("err", 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.
Shouldn't we finish the span here?
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.
We could do that. My idea was that Close
should always be called, so we better Finish
there.
* Added tracing bucket and cache. Signed-off-by: Peter Štibraný <[email protected]> * Don't finish span before calling function. Signed-off-by: Peter Štibraný <[email protected]> * Added extra details. Signed-off-by: Peter Štibraný <[email protected]> * Configure store to use tracing bucket and cache. Signed-off-by: Peter Štibraný <[email protected]> * Don't use nameless embeds. Signed-off-by: Peter Štibraný <[email protected]> * Comments, headers. Signed-off-by: Peter Štibraný <[email protected]> * Fix compilation error after recent changes. Signed-off-by: Peter Štibraný <[email protected]>
This PR adds bucket and cache interactions to the traces.
This PR also fixes bug in Thanos tracing code, which first finished the span and only then called the function with given span.
Verification
Tested manually, see screenshot with small part of example trace.