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

feat: add ParentOrElseSampler #1279

Merged
merged 9 commits into from
Jul 10, 2020

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jul 6, 2020

Which problem is this PR solving?

Short description of the changes

  • Added ParentOrElseSampler.
  • Renamed ALWAYS_SAMPLER and NEVER_SAMPLER to AlwaysOnSampler and
    AlwaysOffSampler respectively.

@legendecas legendecas added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 6, 2020
Also renamed ALWAYS_SAMPLER and NEVER_SAMPLER to AlwaysOnSampler and
AlwaysOffSampler respectively.
@codecov
Copy link

codecov bot commented Jul 6, 2020

Codecov Report

Merging #1279 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1279      +/-   ##
==========================================
+ Coverage   93.12%   93.16%   +0.03%     
==========================================
  Files         133      136       +3     
  Lines        3841     3862      +21     
  Branches      785      788       +3     
==========================================
+ Hits         3577     3598      +21     
  Misses        264      264              
Impacted Files Coverage Δ
...lemetry-core/src/trace/sampler/AlwaysOffSampler.ts 100.00% <100.00%> (ø)
...elemetry-core/src/trace/sampler/AlwaysOnSampler.ts 100.00% <100.00%> (ø)
...etry-core/src/trace/sampler/ParentOrElseSampler.ts 100.00% <100.00%> (ø)
...metry-core/src/trace/sampler/ProbabilitySampler.ts 82.35% <100.00%> (-1.86%) ⬇️
packages/opentelemetry-tracing/src/config.ts 100.00% <100.00%> (ø)

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm,
with regards to description I would prefer having separate unit tests for that only so it is easier to spot the spec requirement for it whenever something fails.

@legendecas
Copy link
Member Author

@obecny The sampler's description is depends on the parameters they were constructed, e.g. for ProbabilitySamplers their description can be ProbabilitySampler{1.0} or ProbabilitySampler{0.5}, and for ParentOrElseSamplers their description can be ParentOrElseSampler{ProbabilitySampler{1.0}} or ParentOrElseSampler{AlwaysOffSampler}. So separating their description assertion from their parameterized tests may cause verbosity.

@obecny
Copy link
Member

obecny commented Jul 8, 2020

@obecny The sampler's description is depends on the parameters they were constructed, e.g. for ProbabilitySamplers their description can be ProbabilitySampler{1.0} or ProbabilitySampler{0.5}, and for ParentOrElseSamplers their description can be ParentOrElseSampler{ProbabilitySampler{1.0}} or ParentOrElseSampler{AlwaysOffSampler}. So separating their description assertion from their parameterized tests may cause verbosity.

It really doesn't matter. The unit tests are meant to test one thing so if some test fails you know exactly what happens. Knowing that if you create a unit test

it('should have correct description', ()=> {
....
}) 

whenever it fails you know exactly that there is a problem with description and you know where to look.

But you created a unit test

it('should return api.SamplingDecision.RECORD_AND_SAMPLED for AlwaysOnSampler'

so if it fails you have no idea why it failed whether it was sampling decision or something different in this case the description.
Also checking something more in certain unit test when the title says 'should return api.SamplingDecision.RECORD_AND_SAMPLED for AlwaysOnSampler is imho wrong, you should simply create a new unit test for that with a proper title.

@obecny
Copy link
Member

obecny commented Jul 8, 2020

should return api.SamplingDecision.NOT_RECORD for not sampled parent while composited with AlwaysOnSampler
imho it should looks like this

describe('when parent is not sampled', ()=> {
  describe('AND composited with AlwaysOnSampler',()=>{
    it('should return api.SamplingDecision.NOT_RECORD', ()=> {
    });
    it('should have correct description', ()=> {
    });
  })
  describe('AND composited with ...',()=>{
.....
})

etc.

@legendecas
Copy link
Member Author

@obecny 👋 , tests updated, PTAL :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace ALWAYS_PARENT sampler with ParentOrElse sampler
6 participants