rule based sampling decisions use the "user" priority values #205
Conversation
test/sample_test.cpp
Outdated
| auto span = tracer->StartSpanWithOptions("operation name", span_options); | ||
| span->FinishWithOptions(finish_options); | ||
| // The first trace will be allowed by the limiter. | ||
| { |
There was a problem hiding this comment.
I'd consider skipping the checks on the first span, as it's covered by another test and isn't the focus point of this specific test.
There was a problem hiding this comment.
Good point, will change.
| // A sampling rule applies to (matches) the current span. | ||
| // | ||
| // Whatever sampling decision we make here (keep or drop) will be of "user" | ||
| // type, i.e. `SamplingPriority::UserKeep` or `SamplingPriority::UserDrop`. |
There was a problem hiding this comment.
Any special reason for markdown-ish backticks in comments? Just curious.
There was a problem hiding this comment.
This was the convention in some code I've worked on before, and generally I like markdown, and especially I like to call out this-is-a-literal-sequence-of-characters-significant-in-code with markup.
The BDE style, probably the fussiest coding standard I've ever had to adhere to, uses a comment markup that uses single quotes for this instead of backticks. That always bothered me. This is also where I get my mechanical function contract language: "Return a blah using the specified blah blah. The behavior is undefined unless blah blah blah."
I notice that our comments don't escape identifiers, so I've been going against the grain. What do you think?
Alternative to this, I also like the Go style, where they make a point of not using markup of any kind. Still, I feel like it is sometimes awkward to talk about a foo::DingleBertFactory instead of a foo::DingleBertFactory.
dgoffredo
force-pushed
the
david.goffredo/rule-based-sampling-is-user-priority
branch
from
a034262 to
1367df2
Compare
dgoffredo
deleted the
david.goffredo/rule-based-sampling-is-user-priority
branch
This is the C++ analog of work done here. These changes supersede the previously discussed addition of new {3, -3} sampling priorities. The new sampling priority values idea didn't work out because some tracer libraries (C++ and PHP) validate sampling priority values, rejecting unknown values. We can't add values in a backward compatible way.
The immediate goal is to prevent the agent from overriding sampling decisions made based on sampling rules. Eventually there will be a new data structure associated with each trace at the root, and information like "who made the sampling decision" will go there. Until that work is done, though, as an interim measure we'll change the sampling priority of sampling rule based decisions. Currently they are "sampler" type, i.e. priority {0, 1}. The agent can override these. Instead, we change them to "user" type, i.e. priority {2, -1}. The agent will not override these.