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.

Sign up for GitHub

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

Jump to bottom

Update Sampler interface base on OTEP 0006-sampling #296

Merged
merged 7 commits into from
Oct 15, 2019
Merged

Update Sampler interface base on OTEP 0006-sampling #296

merged 7 commits into from
Oct 15, 2019

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Oct 10, 2019 •
edited
Loading

Fixes #87
Fixes #33

SergeyKanzhelev
SergeyKanzhelev suggested changes Oct 10, 2019
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

see comments

yurishkuro
yurishkuro reviewed Oct 10, 2019
View reviewed changes
specification/sdk-tracing.md
sampling `Decision` based on information that is typically available just before
the `Span` was created.
sampling `SamplingResult` based on information that is typically available just
before the `Span` was created.

#### ShouldSample

Returns the sampling Decision for a `Span` to be created.

**Required arguments:**
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to generalize this, in light of #31? In Jaeger we recently extended the API to allow sampling decisions to be made later in the span life cycle

  onCreateSpan(span: Span): SamplingDecision
  onSetOperationName(span: Span, operationName: string): SamplingDecision
  onSetTag(span: Span, key: string, value: any): SamplingDecision

Copy link
Member Author

Choose a reason for hiding this comment

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

I think these are moments when the Sampler is called from the SDK perspective. I do agree that this is a nice to have config. Filed #307

@bogdandrutu bogdandrutu mentioned this pull request Oct 11, 2019
Open
@bogdandrutu
Update Sampler interface base on OTEP 0006-sampling
2462ab3 
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu bogdandrutu added this to the Alpha v0.2 milestone Oct 14, 2019
@bogdandrutu
Fix comments from review
74f1499 
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
jkwatson
jkwatson reviewed Oct 14, 2019
View reviewed changes
specification/sdk-tracing.md
arguments collection. Description MUST be `NeverSampleSampler`.
These are the default samplers implemented in the OpenTelemetry SDK:

* ALWAYS_ON
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this isn't a particularly clear name. Can we make it ALWAYS_RETAIN and the other be ALWAYS_DROP ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Please file an issue to not forget about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

#314 done!

SergeyKanzhelev
SergeyKanzhelev approved these changes Oct 14, 2019
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

please apply suggested changes

bogdandrutu and others added 5 commits October 15, 2019 08:10
@bogdandrutu @SergeyKanzhelev
Update specification/sdk-tracing.md
12ce787 
Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>
@bogdandrutu @SergeyKanzhelev
Update specification/sdk-tracing.md
347382c 
Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>
@bogdandrutu @SergeyKanzhelev
Update specification/sdk-tracing.md
43ae6cc 
Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>
@bogdandrutu @SergeyKanzhelev
Update specification/sdk-tracing.md
c48b17b 
Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>
@bogdandrutu
Merge branch 'master' into sampler
520aa12
@bogdandrutu
Copy link
Member Author

@yurishkuro please take another look.

@SergeyKanzhelev
Copy link
Member

This change was discussed in OTEP and hasn't received any requests for change. It also satisfied the requirement on # of approvals and almost a day passed since the last approval. No semantics changes were made in 5 days.

So looks OK to merge for me. Merging

@SergeyKanzhelev SergeyKanzhelev merged commit f1c4f68 into open-telemetry:master Oct 15, 2019
@fbogsany fbogsany mentioned this pull request Oct 16, 2019
Merged
@c24t c24t mentioned this pull request Oct 17, 2019
Merged
SergeyKanzhelev added a commit to SergeyKanzhelev/opentelemetry-specification that referenced this pull request Feb 18, 2020
@bogdandrutu @SergeyKanzhelev
Update Sampler interface base on OTEP 0006-sampling (open-telemetry#296)
fd67250 
* Update Sampler interface base on OTEP 0006-sampling

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

* Fix comments from review

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

* Update specification/sdk-tracing.md

Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>

* Update specification/sdk-tracing.md

Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>

* Update specification/sdk-tracing.md

Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>

* Update specification/sdk-tracing.md

Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>
TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this pull request Oct 15, 2020
@nadiaciobanu
Add OTLP exporter example (open-telemetry#296)
eb04081
@bogdandrutu bogdandrutu deleted the sampler branch February 11, 2021 16:38
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@bogdandrutu @SergeyKanzhelev
Update Sampler interface base on OTEP 0006-sampling (open-telemetry#296)
a97c4c1 
* Update Sampler interface base on OTEP 0006-sampling

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

* Fix comments from review

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

* Update specification/sdk-tracing.md

Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>

* Update specification/sdk-tracing.md

Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>

* Update specification/sdk-tracing.md

Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>

* Update specification/sdk-tracing.md

Co-Authored-By: Sergey Kanzhelev <S.Kanzhelev@live.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkwatson jkwatson jkwatson left review comments

@yurishkuro yurishkuro yurishkuro left review comments

@carlosalberto carlosalberto carlosalberto approved these changes

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@AloisReitbauer AloisReitbauer Awaiting requested review from AloisReitbauer

@c24t c24t Awaiting requested review from c24t

@iredelmeier iredelmeier Awaiting requested review from iredelmeier

@reyang reyang Awaiting requested review from reyang

@songy23 songy23 Awaiting requested review from songy23

@tedsuo tedsuo Awaiting requested review from tedsuo

@tigrannajaryan tigrannajaryan Awaiting requested review from tigrannajaryan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Alpha v0.2
Development

Successfully merging this pull request may close these issues.

API: Provided Samplers in the SDK Document that default sampler is 100% sampler
5 participants
@bogdandrutu @SergeyKanzhelev @carlosalberto @jkwatson @yurishkuro