Expose tunning options via expvar #2496

dstdfx · 2020-09-21T06:50:45Z

Which problem is this PR solving?

Resolves Expose the tuning options via expvar #1333

Short description of the changes

Mounts expvar handler (/debug/vars) on the admin port regardless of --metrics-backend
Stores the following collector's options in expvar:
- collector.num-workers
- collector.queue-size
Store the following agent's options in expvar:
- processor.*.server-max-packet-size
- processor.*.server-queue-size
- processor.*.workers
Store the following storage-related options in expvar:
- memory.max-traces
- downsampling.ratio
- span-storage.type

dstdfx · 2020-09-21T08:01:01Z

@jpkrohling is it CI-related issue https://travis-ci.org/github/jaegertracing/jaeger/jobs/728900004 ?

jpkrohling · 2020-09-21T08:09:24Z

This test has indeed been flaky...

codecov · 2020-09-21T08:18:37Z

Codecov Report

Merging #2496 into master will decrease coverage by 0.03%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master    #2496      +/-   ##
==========================================
- Coverage   95.36%   95.33%   -0.04%     
==========================================
  Files         208      208              
  Lines        9222     9248      +26     
==========================================
+ Hits         8795     8817      +22     
- Misses        350      354       +4     
  Partials       77       77

Impacted Files	Coverage Δ
cmd/collector/app/builder_flags.go	`100.00% <ø> (ø)`
cmd/flags/service.go	`0.00% <0.00%> (ø)`
cmd/agent/app/builder.go	`95.89% <100.00%> (+0.57%)`	⬆️
cmd/agent/app/flags.go	`91.66% <100.00%> (ø)`
cmd/collector/app/collector.go	`69.44% <100.00%> (+1.79%)`	⬆️
plugin/storage/factory.go	`98.07% <100.00%> (+0.11%)`	⬆️
plugin/storage/memory/factory.go	`100.00% <100.00%> (ø)`
...lugin/sampling/strategystore/adaptive/processor.go	`99.07% <0.00%> (-0.93%)`	⬇️
plugin/storage/integration/integration.go	`77.90% <0.00%> (+0.55%)`	⬆️

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 4f3b832...e877351. Read the comment docs.

jpkrohling · 2020-09-21T08:19:32Z

Tests are passing now :-)

jpkrohling

I like the base idea, but I think the actual registration with expvar could be closer to where the values are used or defined.

I would also love to see other values, although they might come in a later PR:

--memory.max-traces
--span-storage.type (not sure the storate type is actually stored there)
--downsampling.ratio

cmd/agent/app/flags_test.go

cmd/agent/main.go

jpkrohling

Looks good to me. One thing that we might want to add is the storage type. What do you think?

dstdfx · 2020-09-22T09:40:56Z

Sure, wilco

dstdfx · 2020-09-22T13:26:21Z

@jpkrohling Is it known issue https://travis-ci.org/github/jaegertracing/jaeger/jobs/729295839?

jpkrohling · 2020-09-22T14:57:35Z

Doesn't sound like it, looks like it's a real problem with the new test:

github.com/jaegertracing/jaeger/plugin/storage.TestSetExpvarOptions(0xc000229080)
	/home/travis/gopath/src/github.com/jaegertracing/jaeger/plugin/storage/factory_test.go:364 +0x337

dstdfx · 2020-09-22T15:59:54Z

Doesn't sound like it, looks like it's a real problem with the new test:

github.com/jaegertracing/jaeger/plugin/storage.TestSetExpvarOptions(0xc000229080)
	/home/travis/gopath/src/github.com/jaegertracing/jaeger/plugin/storage/factory_test.go:364 +0x337

My bad, didn't notice...

cmd/agent/app/builder.go

cmd/agent/main.go

cmd/opentelemetry/go.mod

plugin/storage/factory.go

Always mount expvar /debug/vars handler regardless of chosen metrics backend. Store the following collector's options in expvar: - collector.num-workers - collector.queue-size Store the following agent's options in expvar: - processor.*.server-max-packet-size - processor.*.server-queue-size - processor.*.workers Signed-off-by: Daniil Rutskiy <[email protected]>

Signed-off-by: Daniil Rutskiy <[email protected]>

Put expvar logic into private funcs of builders. Use different values in tests. Signed-off-by: Daniil Rutskiy <[email protected]>

Store following storage-related options in expvar: - memory.max-traces - downsampling.ratio Signed-off-by: Daniil Rutskiy <[email protected]>

Signed-off-by: Daniil Rutskiy <[email protected]>

Delete direct expvar calls. Vendor latest jaeger-lib. Signed-off-by: Daniil Rutskiy <[email protected]>

Signed-off-by: Daniil Rutskiy <[email protected]>

yurishkuro

Looks great! The only question is about otel/go.mod

cmd/opentelemetry/go.mod

yurishkuro · 2020-10-05T18:09:03Z

I am going to merge it, please put another PR for otel/go.mod

dstdfx requested a review from a team as a code owner September 21, 2020 06:50

dstdfx requested a review from jpkrohling September 21, 2020 06:50

jpkrohling self-assigned this Sep 21, 2020

jpkrohling reviewed Sep 21, 2020

View reviewed changes

cmd/agent/app/flags_test.go Outdated Show resolved Hide resolved

cmd/agent/main.go Outdated Show resolved Hide resolved

dstdfx force-pushed the expose-opts-via-expvar branch from 6f6a216 to 0bd8431 Compare September 22, 2020 09:31

jpkrohling reviewed Sep 22, 2020

View reviewed changes

dstdfx force-pushed the expose-opts-via-expvar branch from ce2eb02 to 2231f49 Compare September 22, 2020 12:41

yurishkuro reviewed Sep 22, 2020

View reviewed changes

cmd/agent/app/builder.go Outdated Show resolved Hide resolved

dstdfx mentioned this pull request Sep 24, 2020

Implement fork.Factory jaegertracing/jaeger-lib#84

Merged

dstdfx force-pushed the expose-opts-via-expvar branch from d8913fb to e1184dc Compare September 28, 2020 07:53

dstdfx requested a review from yurishkuro September 30, 2020 08:40