Skip to content
This repository was archived by the owner on Jul 1, 2022. It is now read-only.

Conversation

@isaachier
Copy link
Contributor

Signed-off-by: Isaac Hier ihier@uber.com

Which problem is this PR solving?

Short description of the changes

  • Moves SamplingStatus from internal package to spi package.

@codecov
Copy link

codecov bot commented Jul 22, 2018

Codecov Report

Merging #502 into master will decrease coverage by 1.55%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #502      +/-   ##
===========================================
- Coverage     89.76%   88.2%   -1.56%     
+ Complexity      563     498      -65     
===========================================
  Files            69      65       -4     
  Lines          2061    1848     -213     
  Branches        261     239      -22     
===========================================
- Hits           1850    1630     -220     
- Misses          132     141       +9     
+ Partials         79      77       -2
Impacted Files Coverage Δ Complexity Δ
...main/java/io/jaegertracing/spi/SamplingStatus.java 66.66% <ø> (ø) 2 <0> (?)
...racing/internal/samplers/ProbabilisticSampler.java 81.81% <ø> (ø) 12 <0> (ø) ⬇️
...internal/samplers/GuaranteedThroughputSampler.java 92.3% <ø> (ø) 7 <0> (ø) ⬇️
...ing/internal/samplers/RemoteControlledSampler.java 89.53% <ø> (-1.72%) 17 <0> (+1)
...tracing/internal/samplers/RateLimitingSampler.java 66.66% <ø> (ø) 6 <0> (ø) ⬇️
...tracing/internal/samplers/PerOperationSampler.java 93.47% <ø> (ø) 18 <0> (ø) ⬇️
.../jaegertracing/internal/samplers/ConstSampler.java 66.66% <ø> (ø) 4 <0> (ø) ⬇️
...n/java/io/jaegertracing/internal/JaegerTracer.java 89.83% <ø> (+0.86%) 24 <0> (-2) ⬇️
...rtracing/internal/reporters/CompositeReporter.java 71.42% <0%> (-28.58%) 5% <0%> (-2%)
...java/io/jaegertracing/zipkin/ZipkinV2Reporter.java 71.42% <0%> (-28.58%) 2% <0%> (-1%)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3947cbf...961c74b. Read the comment docs.

Signed-off-by: Isaac Hier <ihier@uber.com>
@isaachier isaachier force-pushed the move-sampling-status branch from 347626e to 961c74b Compare August 3, 2018 16:50
@isaachier
Copy link
Contributor Author

@yurishkuro @jpkrohling @vprithvi any thoughts about this change?

@yurishkuro
Copy link
Member

Long term I think this is the right change. Short term I'd prefer not to do it, because the whole point of package renaming was to give us the opportunity to revisit & clean up the interfaces. In particular, returning the status objects is not very efficient as it requires memory allocation. Also, Won and I discussed that we may want to change the sampler signature to be more efficient, in particular by accepting the current time stamp (used by the rate limiter), or the whole span that already contains the time stamp, the span id, and the operation name (but passing the span creates a dependency loop of sorts).

So I would've actually preferred not having the Sampler interface in the spi package just yet.

Why not making this change blocks you from implementing a Sampler? The internal package is still accessible.

@isaachier
Copy link
Contributor Author

I don't believe I need this at the moment. Just checking in. Should I close this or leave it open just in case we want this change later on?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move SamplingStatus out of internal package

2 participants