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

Allow spans to be sampled client-side (head-based sampling) #2

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

Conversation

pontello
Copy link
Collaborator

@pontello pontello commented Dec 1, 2022

Allow sampling to be configured with minimal changes to the upstream code.

The idea is to maintain this fork and keep pulling updates till lightstep#306 is resolved.

@pontello pontello self-assigned this Dec 1, 2022
@pontello pontello marked this pull request as ready for review December 1, 2022 14:18
@pontello pontello changed the title Allow spans to be sampled client-side (head-based sampling) - V2 Allow spans to be sampled client-side (head-based sampling) Dec 1, 2022
@nick-kentik
Copy link

Just copying a discussion from slack:

Nick Thomas
V2 is smaller, but also has no chance of being accepted upstream, and while there's a chance of that, I think it make more sense to have one live changeset than two. V2 would definitely be better if we were maintaining this long-term, for the reasons you outline.
We don't use the publicly accessible WithSpan* methods anywhere, and I think it's unlikely we would.
WDYT to sticking with V1 until jan, which is when I'm told they'll next look at upstreaming this. If it's a no then, we can switch to V2 and assume long-term maintenance, and/or sort out getting something that ditches launcher into kit. I don't expect there to be an urgent need to upgrade over december, so delaying isn't likely to cost us anything, is it?

nilson
You are right.
The V2 is preferred for long term if they don’t solve our sampling issues somehow, either via your PR or with server-side sampling.
I will keep only V2 PR for kentik fork and we re-evaluate by end of January, ok?

Nick Thomas
sounds good to me :+1:

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.

2 participants