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

Flush the LS tracer, since Close() doesn't seem to. #206

Merged
merged 4 commits into from
Jul 18, 2017

Conversation

cory-stripe
Copy link
Contributor

@cory-stripe cory-stripe commented Jul 18, 2017

Summary

Flush spans before we close the tracer.

Motivation

Based on my read of the LS tracer code, it doesn't actually flush on close. This means that if you stuff a bunch of spans in really fast and call Close() you're dropping things.

I based this on this code that calls this code none of which calls Flush anywhere.

It's my theory that our recent deploy of Veneur to prod has actually broken tracing becuase we're not flushing before close.

While were here, this adds a metric that tracks the duration of each flush so we can see how much we're impacting things.

Test plan

Validating in QA and testing on a single prod apibox, since this is where the problem was manifested.

Rollout/monitoring/revert plan

Merge, make PR for this SHA in puppet-land and test a single box.

r? @stripe/observability

@stripe-ci
Copy link

Gerald Rule: Copy Observability on Veneur and Unilog pull requests

cc @stripe/observability
cc @stripe/observability-stripe

@joshu-stripe
Copy link
Contributor

Looks good to me!

I’m curious why lightstep isn’t flushing these themselves, and may do a little digging to see if I can find out.

@cory-stripe cory-stripe force-pushed the cory-flush-ls-tracer branch from 6975298 to d399cb3 Compare July 18, 2017 17:04
@cory-stripe
Copy link
Contributor Author

re-approving for CL entry and rebase

@cory-stripe cory-stripe merged commit fa157ec into master Jul 18, 2017
@cory-stripe cory-stripe deleted the cory-flush-ls-tracer branch July 18, 2017 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants