Skip to content

Use the default metricsFactory if not provided #739

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 14, 2018
Merged

Use the default metricsFactory if not provided #739

merged 1 commit into from
Mar 14, 2018

Conversation

ledor473
Copy link
Member

This will fix #738

I'm looking at adding a test to ensure the functionality will stay over time. I'll update the PR

@coveralls
Copy link

coveralls commented Mar 14, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling e58bfe5 on Ticketmaster:fix-default-metrics-factory into 6fce4f9 on jaegertracing:master.

@ledor473
Copy link
Member Author

A test case was added, but an error is returned when withRunningAgent is used twice:

panic: duplicate metrics collector registration attempted [recovered]
	panic: duplicate metrics collector registration attempted

goroutine 73 [running]:
testing.tRunner.func1(0xc420294000)
	/usr/local/Cellar/go/1.10/libexec/src/testing/testing.go:742 +0x29d
panic(0x15a33e0, 0xc4202404c0)
	/usr/local/Cellar/go/1.10/libexec/src/runtime/panic.go:505 +0x229
github.com/jaegertracing/jaeger/vendor/github.com/prometheus/client_golang/prometheus.(*Registry).MustRegister(0xc42001ea00, 0xc420258210, 0x1, 0x1)
	/Users/ledor473/go/src/github.com/jaegertracing/jaeger/vendor/github.com/prometheus/client_golang/prometheus/registry.go:404 +0x9e
github.com/jaegertracing/jaeger/vendor/github.com/uber/jaeger-lib/metrics/prometheus.(*vectorCache).getOrMakeCounterVec(0xc420244480, 0x0, 0x0, 0x0, 0x0, 0xc420270200, 0x31, 0xc420270200, 0x31, 0x0, ...)
	/Users/ledor473/go/src/github.com/jaegertracing/jaeger/vendor/github.com/uber/jaeger-lib/metrics/prometheus/cache.go:50 +0x213
github.com/jaegertracing/jaeger/vendor/github.com/uber/jaeger-lib/metrics/prometheus.(*Factory).Counter(0xc420242440, 0xc420270200, 0x31, 0xc4202447e0, 0x0, 0x0)
	/Users/ledor473/go/src/github.com/jaegertracing/jaeger/vendor/github.com/uber/jaeger-lib/metrics/prometheus/factory.go:110 +0x186
github.com/jaegertracing/jaeger/vendor/github.com/uber/jaeger-lib/metrics.initMetrics(0x1514200, 0xc42026e280, 0x168c720, 0xc420242440, 0x0, 0x50, 0x15c5e40)
	/Users/ledor473/go/src/github.com/jaegertracing/jaeger/vendor/github.com/uber/jaeger-lib/metrics/metrics.go:72 +0x43e
github.com/jaegertracing/jaeger/vendor/github.com/uber/jaeger-lib/metrics.Init(0x1514200, 0xc42026e280, 0x168c720, 0xc420242440, 0x0)
	/Users/ledor473/go/src/github.com/jaegertracing/jaeger/vendor/github.com/uber/jaeger-lib/metrics/metrics.go:25 +0x57
github.com/jaegertracing/jaeger/cmd/agent/app/reporter/tchannel.New(0x161f664, 0x10, 0xc420298000, 0x0, 0x168c720, 0xc420242300, 0xc42024e600, 0xc420242301)
	/Users/ledor473/go/src/github.com/jaegertracing/jaeger/cmd/agent/app/reporter/tchannel/reporter.go:78 +0x283
github.com/jaegertracing/jaeger/cmd/agent/app/reporter/tchannel.(*Builder).CreateReporter(0xc4201ecee8, 0x168c720, 0xc420242300, 0xc42024e600, 0xc420242300, 0x0, 0x0)
	/Users/ledor473/go/src/github.com/jaegertracing/jaeger/cmd/agent/app/reporter/tchannel/builder.go:119 +0x142
github.com/jaegertracing/jaeger/cmd/agent/app.(*Builder).createMainReporter(0xc4201ece90, 0x168c720, 0xc420242300, 0xc42024e600, 0x0, 0xc4202401c0, 0xc420278140)
	/Users/ledor473/go/src/github.com/jaegertracing/jaeger/cmd/agent/app/builder.go:113 +0x4f
github.com/jaegertracing/jaeger/cmd/agent/app.(*Builder).CreateAgent(0xc420077e90, 0xc42024e600, 0x2, 0x2, 0x0)
	/Users/ledor473/go/src/github.com/jaegertracing/jaeger/cmd/agent/app/builder.go:129 +0x8b
github.com/jaegertracing/jaeger/cmd/agent/app.withRunningAgent(0xc420294000, 0xc420077f88)
	/Users/ledor473/go/src/github.com/jaegertracing/jaeger/cmd/agent/app/agent_test.go:101 +0x150
github.com/jaegertracing/jaeger/cmd/agent/app.TestAgentMetricsEndpoint(0xc420294000)
	/Users/ledor473/go/src/github.com/jaegertracing/jaeger/cmd/agent/app/agent_test.go:71 +0x46

@ledor473 ledor473 force-pushed the fix-default-metrics-factory branch from 8bf18e5 to c06df3f Compare March 14, 2018 19:41
@ledor473 ledor473 changed the title WIP: Use the provided metricsFactory or default if none is provided Use the default metricsFactory if not provided Mar 14, 2018
@ledor473
Copy link
Member Author

The test is working now (had to re-create the prometheus.DefaultRegistry before each test).
I've also squash my commits and add the sign-off needed for the DCO

@black-adder
Copy link
Contributor

black-adder commented Mar 14, 2018 •
edited
Loading

looks like travis fails due to formatting, run make fmt

@ledor473 ledor473 force-pushed the fix-default-metrics-factory branch from c06df3f to efb5882 Compare March 14, 2018 19:57
@ledor473
Copy link
Member Author

All green now! 👍

ledor473
ledor473 commented Mar 14, 2018
View reviewed changes
cmd/agent/app/agent_test.go
@@ -51,6 +94,10 @@ func TestAgentStartStop(t *testing.T) {
HTTPServer: HTTPServerConfiguration{
HostPort: ":0",
},
Metrics: jmetrics.Builder{
Backend: "prometheus",
HTTPRoute: "/metrics",
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to place the default values here as they are usually initialized using Viper

yurishkuro
yurishkuro approved these changes Mar 14, 2018
View reviewed changes
cmd/agent/app/agent_test.go Outdated
}

func withRunningAgent(t *testing.T, testcase func(string, chan error)) {
withNewPrometheusRegistry()
Copy link
Member

Choose a reason for hiding this comment

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

nit: resetDefaultPrometheusRegistry

Use the default metricsFactory if not provided
e58bfe5 
Signed-off-by: Louis-Etienne Dorval <louis-etienne.dorval@ticketmaster.com>
@ledor473 ledor473 force-pushed the fix-default-metrics-factory branch from efb5882 to e58bfe5 Compare March 14, 2018 20:44
@yurishkuro yurishkuro merged commit ff666cd into jaegertracing:master Mar 14, 2018
@yurishkuro
Copy link
Member

Thanks for the fix @ledor473

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

@yurishkuro yurishkuro yurishkuro approved these changes

@black-adder black-adder Awaiting requested review from black-adder

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@vprithvi vprithvi Awaiting requested review from vprithvi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Jaeger Agent metrics not returned
4 participants
@ledor473 @coveralls @black-adder @yurishkuro