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

Refer to valid sampler type values (doc link) #557

Merged
merged 2 commits into from
Oct 16, 2018

Conversation

sjoerdtalsma
Copy link
Contributor

Which problem is this PR solving?

  • Make it easier to find the valid sampler types from the java client README

Short description of the changes

  • Link to jaeger documentation on client samling configuration.

@codecov
Copy link

codecov bot commented Oct 15, 2018

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #557      +/-   ##
============================================
- Coverage     89.74%   89.38%   -0.37%     
+ Complexity      540      537       -3     
============================================
  Files            68       68              
  Lines          1940     1940              
  Branches        249      249              
============================================
- Hits           1741     1734       -7     
- Misses          126      132       +6     
- Partials         73       74       +1
Impacted Files Coverage Δ Complexity Δ
...rtracing/internal/reporters/CompositeReporter.java 71.42% <0%> (-28.58%) 6% <0%> (-1%)
...java/io/jaegertracing/zipkin/ZipkinV2Reporter.java 71.42% <0%> (-28.58%) 2% <0%> (-1%)
...gertracing/internal/reporters/LoggingReporter.java 81.81% <0%> (-9.1%) 4% <0%> (-1%)

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 19a5110...ce8cb0e. Read the comment docs.

@@ -86,7 +86,7 @@ JAEGER_PROPAGATION | no | Comma separated list of formats to use for propagating
JAEGER_REPORTER_LOG_SPANS | no | Whether the reporter should also log the spans
JAEGER_REPORTER_MAX_QUEUE_SIZE | no | The reporter's maximum queue size
JAEGER_REPORTER_FLUSH_INTERVAL | no | The reporter's flush interval (ms)
JAEGER_SAMPLER_TYPE | no | The sampler type
JAEGER_SAMPLER_TYPE | no | [The sampler type](https://www.jaegertracing.io/docs/latest/sampling/#client-sampling-configuration)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea, but think it might be better if the hyperlink was just on the words "sampler type".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Updated the link

Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

LGTM - can you sign the comment though. Thanks.

@sjoerdtalsma
Copy link
Contributor Author

@objectiser

LGTM - can you sign the comment though. Thanks.

Sure, want me to just add a "Signed off by" line to the main comment of this PR or must I ammend my commits? (I made the change through the github web UI, so not that comfortable here 😉 )

@objectiser
Copy link
Contributor

@sjoerdtalsma You will need to amend the commit, you could squash and rebase at the same time.

@sjoerdtalsma
Copy link
Contributor Author

-> Any idea how to do this through github's UI?

The commits are signed by github because I use 2PA I think (see Verified with the commits).
Unfortunately I can't sign locally, as I don't have my PGP keys with me on the computer I'm currently using.

@sjoerdtalsma
Copy link
Contributor Author

I could re-do the change this evening on my laptop though, so don't spend too much time on this 😉

Signed-off-by: Sjoerd Talsma <sjoerd@talsma-ict.nl>
@sjoerdtalsma
Copy link
Contributor Author

Rebased, squashed, amended, signed, signed off and force-pushed... 😄

@sjoerdtalsma
Copy link
Contributor Author

Travis build failed??

@ghost ghost assigned pavolloffay Oct 16, 2018
@ghost ghost added the review label Oct 16, 2018
@objectiser objectiser merged commit 10c641f into jaegertracing:master Oct 16, 2018
@ghost ghost removed the review label Oct 16, 2018
@sjoerdtalsma sjoerdtalsma deleted the patch-2 branch October 16, 2018 14:05
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.

3 participants