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

rework over Brave 5.11 (or 5.12) #31

Open
codefromthecrypt opened this issue Apr 12, 2020 · 0 comments
Open

rework over Brave 5.11 (or 5.12) #31

codefromthecrypt opened this issue Apr 12, 2020 · 0 comments

Comments

@codefromthecrypt
Copy link
Contributor

The code here is currently doing more things than it needs to, as it should be possible to leverage the baggage apis in 5.11+. Currently, the main thing needed is to influence the localSampled decision.

Another option is to not do that.. if we accept turning local sampling on 100% of the time, the design gets very easy.

Right now, when we either know this root is secondary sampling, or think it might be, we locally sample the span. If we thought it might be, but it ends up not needed, we just drop it. If we always sample local, then we don't need to intercept context creation. The tradeoff is needless overhead when no secondary sampling occurs (in process). OTOH if there's any other listener for all spans, like metrics, that wouldn't be needless overhead as the other consumer would still use the data collected.

Basically the design is simpler if we always sample locally, but there's more GC churn for possibly no benefit. I'm inclined to keep the context hook to avoid that, but it is a decision worth re-thinking now that there's some practice.

@narayaruna any thoughts on this or experience to share? cc also @anuraaga

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

No branches or pull requests

1 participant