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

Implement fork.Factory #84

Merged
merged 7 commits into from
Sep 25, 2020
Merged

Conversation

dstdfx
Copy link
Contributor

@dstdfx dstdfx commented Sep 24, 2020

Which problem is this PR solving?

Short description of the changes

  • Implement fork.Factory that delegates metrics with
    specific namespaces to different factory, otherwise
    default factory is used.

Implement fork.Factory that deligates metrics with
specific namespaces to different factory, otherwise
default factory is used.

Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>
Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>
@codecov
Copy link

codecov bot commented Sep 24, 2020

Codecov Report

Merging #84 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #84   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        27    +1     
  Lines          695       709   +14     
=========================================
+ Hits           695       709   +14     
Impacted Files Coverage Δ
metrics/fork/fork.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 48cc1df...9baf8df. Read the comment docs.

Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>
@dstdfx
Copy link
Contributor Author

dstdfx commented Sep 24, 2020

cc @yurishkuro

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.

lgtm, a few small comments

metrics/fork/fork_test.go Outdated Show resolved Hide resolved
metrics/fork/fork_test.go Outdated Show resolved Hide resolved
"github.com/uber/jaeger-lib/metrics/go-kit",
"github.com/uber/jaeger-lib/metrics/go-kit/expvar",
"github.com/uber/jaeger-lib/metrics/metricstest",
"github.com/uber/jaeger-lib/metrics/prometheus",
Copy link
Member

Choose a reason for hiding this comment

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

which version of dep are you using? Ideally there should be no dependency changes in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dep:
 version     : v0.5.4
 build date  : 2019-09-29
 git hash    : 1f7c19e
 go version  : go1.13
 go compiler : gc
 platform    : darwin/amd64
 features    : ImportDuringSolve=false

Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>
Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>
Signed-off-by: Daniil Rutskiy <dstdfx@gmail.com>
@yurishkuro yurishkuro merged commit 2cac3b3 into jaegertracing:master Sep 25, 2020
@yurishkuro
Copy link
Member

👍

@dstdfx dstdfx deleted the fork-factory branch September 27, 2020 08:18
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.

2 participants