Finish up configs for tail sampling policies #221
Conversation
songy23
requested review from
bogdandrutu,
flands,
pjanotti and
tigrannajaryan
as code owners
Codecov Report
@@ Coverage Diff @@
## master #221 +/- ##
==========================================
+ Coverage 72.79% 73.52% +0.72%
==========================================
Files 110 110
Lines 6338 6369 +31
==========================================
+ Hits 4614 4683 +69
+ Misses 1491 1450 -41
- Partials 233 236 +3
Continue to review full report at Codecov.
|
songy23
force-pushed
the
tail-sampling-config
branch
4 times, most recently
from
9eef1b4 to
f76be2a
Compare
|
@songy23 the change itself looks good. As you pointed, correctly, it is not possible with the new config (as it is at this moment) to select the exporters/destinations of each police as before. However, I think that there is still value in having multiple policies per instance of tail sampling processor. Did you hit any blocker in that path? If yes we can start with single as in the PR and later make the work to support multiple. |
songy23
force-pushed
the
tail-sampling-config
branch
from
f76be2a to
4f69894
Compare
|
I don't see any blocker here. We can add multiple policies in one processor though they all pass the trace data to the same consumer. |
|
Yes, it will be the same consumer but they still can combine policies so it is closer (not equal) in capability to what we had before. |
songy23
force-pushed
the
tail-sampling-config
branch
from
4f69894 to
7f82247
Compare
|
7f82247 restored the ability on having multiple policies per instance of tail sampling processor. PTAL. |
processor/tailsampling/config.go
Outdated
| // RateLimiting allows all traces until the specified limits are satisfied. | ||
| RateLimiting PolicyType = "rate-limiting" | ||
| // RateLimitingFilter allows all traces until the specified limits are satisfied. | ||
| RateLimitingFilter PolicyType = "rate-limiting-filter" |
There was a problem hiding this comment.
Do we need the suffix -filter? Are their any policies that are not filters? If not I think it would be simpler to just drop it.
There was a problem hiding this comment.
Added because all other policies have the "-filter" suffix. We can remove it from all other configs instead, I don't have a strong preference here.
There was a problem hiding this comment.
I vote for removing from all, unless there is a reason for the -filter prefix. @pjanotti @bogdandrutu what do you think?
There was a problem hiding this comment.
Agreed: the suffix seems unnecessary, I don't recall any reason to have them.
|
Thanks :) |
* Remove the experimental SDK * Remove the experimental SDK from README.md * Remove the experimental SDK from Makefile
Fixes #146.
Note one additional change is now each tail sampling processor can onlyaccept one policy, because one processor can only have one next
consumer. If users need more than one policies they can define multiple
tail sampling processors in the config.