Add rate limited sampling smoke tests for ingestion sampling - #3731
Add rate limited sampling smoke tests for ingestion sampling#3731Trask Stalnaker (trask) wants to merge 2 commits into
Conversation
Sorry, something went wrong.
| assertThat(rdEnvelopes.size()).isEqualTo(1000); | ||
|
|
||
| for (Envelope rdEnvelope : rdEnvelopes) { | ||
| assertThat(rdEnvelope.getSampleRate()).isEqualTo(99.99f); |
There was a problem hiding this comment.
When sampling is disabled shouldn't this be 100%?
Sorry, something went wrong.
There was a problem hiding this comment.
IIRC when it's 100 then ingestion sampling will take effect
but then again, I thought ingestion sampling only takes effect when we send no sample rate, so maybe it can be 100, I'll need to re-test this to confirm
Sorry, something went wrong.
There was a problem hiding this comment.
Looking at this part of code where sample rate is set to 99.99 when sampler returns 100:
. I do see node.js only setting sample rate attr if the rate returned from sampler isn't 100. Probably for the same reason - service side sampling applies at 100 or empty.
Sorry, something went wrong.
There was a problem hiding this comment.
Trask Stalnaker (@trask) you may want to put a comment about why you expect this to be 99.99 in the test so folks don't get confused
Sorry, something went wrong.
| assertThat(rdEnvelopes.size()).isEqualTo(1000); | ||
|
|
||
| for (Envelope rdEnvelope : rdEnvelopes) { | ||
| assertThat(rdEnvelope.getSampleRate()).isNull(); |
There was a problem hiding this comment.
Shouldn't this have a meaningful value when sampling is enabled? Am I missing something here?
Sorry, something went wrong.
There was a problem hiding this comment.
ah, this is about ingestion (server-side) sampling
Sorry, something went wrong.
There was a problem hiding this comment.
Can the name of this test be changed to make this more clear? To my understanding, this test asserts that when rate limited sampler is used, everything is sampled in at 100% & ingestion sampling enabled in json config, then we expect the rate to be null?
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
No description provided.