Skip to content

Add custom sampler project#1001

Merged
cijothomas merged 4 commits intomasterfrom
reyang/sampler
Aug 5, 2020
Merged

Add custom sampler project#1001
cijothomas merged 4 commits intomasterfrom
reyang/sampler

Conversation

@reyang
Copy link
Copy Markdown
Member

@reyang reyang commented Aug 4, 2020

This is very similar to #996.
Trying to use the docs and tutorial projects to drive for better design and implementation.
This change should give us some idea about the sampler use case.

Changes

  • Added custom sampler tutorial project

@reyang reyang requested a review from a team August 4, 2020 19:48
@reyang reyang added the documentation Documentation related label Aug 4, 2020
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 4, 2020

Codecov Report

Merging #1001 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1001   +/-   ##
=======================================
  Coverage   68.42%   68.42%           
=======================================
  Files         220      220           
  Lines        5989     5989           
  Branches      981      981           
=======================================
  Hits         4098     4098           
+ Misses       1619     1617    -2     
- Partials      272      274    +2     
Impacted Files Coverage Δ
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 80.88% <0.00%> (-5.89%) ⬇️
src/OpenTelemetry/Metrics/CounterMetricSdkBase.cs 93.33% <0.00%> (+13.33%) ⬆️

@reyang reyang closed this Aug 4, 2020
@reyang reyang reopened this Aug 4, 2020
Copy link
Copy Markdown
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM! Issue with the sampler is we only currently allow true/false response but by the spec there should be 3 states?

@reyang
Copy link
Copy Markdown
Member Author

reyang commented Aug 5, 2020

LGTM! Issue with the sampler is we only currently allow true/false response but by the spec there should be 3 states?

That's already showing the value of having these simple tutorials - if we're unhappy with the tutorial projects, we know we got to fix something 😄.

@cijothomas
Copy link
Copy Markdown
Member

LGTM! Issue with the sampler is we only currently allow true/false response but by the spec there should be 3 states?

It's tracked as an issue tagged p1 for coming release. @rajkumar-rangaraj will be working on it

@cijothomas cijothomas merged commit 0bcf363 into master Aug 5, 2020
@cijothomas cijothomas deleted the reyang/sampler branch August 5, 2020 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Documentation related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants