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: extend configuration to support custom randomNumber func #555

Merged

Conversation

guanwenbogit
Copy link
Contributor

Which problem is this PR solving?

  • I want set tracer randomNumber when the config.New() called

Short description of the changes

  • Added the WithRandomNumber function and the randomNumber field for config.Options. Finally, apply the setting in the config.NewTracer() function

@guanwenbogit guanwenbogit changed the title feat: costume tracer randomNumber func on config.New() called feat: customize tracer randomNumber func on config.New() called Dec 18, 2020
@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #555 (4e06150) into master (17fd3e8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #555      +/-   ##
==========================================
+ Coverage   88.59%   88.61%   +0.01%     
==========================================
  Files          61       61              
  Lines        3307     3311       +4     
==========================================
+ Hits         2930     2934       +4     
  Misses        251      251              
  Partials      126      126              
Impacted Files Coverage Δ
config/config.go 95.33% <100.00%> (+0.06%) ⬆️
config/options.go 100.00% <100.00%> (ø)

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 17fd3e8...4e06150. Read the comment docs.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Generally looks fine, but let's add some tests to cover the new functionality.

@@ -147,6 +148,13 @@ func Extractor(format interface{}, extractor jaeger.Extractor) Option {
}
}

// WithRandonNunmber set the Tracer random number func
func WithRandonNunmber(f func() uint64) Option {
Copy link
Member

Choose a reason for hiding this comment

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

nit: WithRandomNumber

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Emmmmmmmm, my mistake.

config/options.go Outdated Show resolved Hide resolved
@yurishkuro
Copy link
Member

Please make sure all commits are signed. Might be easier to squash them into one. See CONTRIBUTING.md.

guanwenbo added 4 commits December 21, 2020 20:14
Signed-off-by: guanwenbo <guanwenbo@inke.cn>
Signed-off-by: guanwenbo <guanwenbo@inke.cn>
Signed-off-by: guanwenbo <guanwenbo@inke.cn>
Signed-off-by: guanwenbo <guanwenbo@inke.cn>
guanwenbo and others added 5 commits December 21, 2020 20:43
Signed-off-by: guanwenbo <guanwenbostar@163.com>
Signed-off-by: guanwenbo <guanwenbostar@163.com>
Signed-off-by: guanwenbo <guanwenbostar@163.com>
Signed-off-by: guanwenbo <guanwenbostar@163.com>
Signed-off-by: guanwenbo <guanwenbostar@163.com>
}
randomNum := func() uint64 { return traceID }
tracer, closer, err := cfg.NewTracer(WithRandomNumber(randomNum))
span:=tracer.StartSpan("test-span")
Copy link
Member

Choose a reason for hiding this comment

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

needs make fmt

spanCtx := span.Context().(jaeger.SpanContext)

assert.NoError(t, err)
assert.Equal(t, spanCtx.TraceID().Low ,traceID)
Copy link
Member

Choose a reason for hiding this comment

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

nit: wrong argument order, expected value (traceID) goes first

Signed-off-by: guanwenbo <guanwenbostar@163.com>
Signed-off-by: guanwenbo <guanwenbostar@163.com>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@yurishkuro yurishkuro changed the title feat: customize tracer randomNumber func on config.New() called feat: extend configuration to support custom randomNumber func Dec 23, 2020
@yurishkuro yurishkuro merged commit fe3fa55 into jaegertracing:master Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants