Add /api/sampling endpoint in collector #1990

RickyRajinder · 2019-12-21T00:12:01Z

Which problem is this PR solving?

Resolves Replicate /sampling endpoint from agent in the collector #1971: Expose the /api/sampling endpoint in collector to allow clients to retrieve sampling strategies without using the agent.

Short description of the changes

Consolidated agent and collector API handlers into one

cmd/collector/app/http_handler.go

cmd/collector/app/http_handler_test.go

cmd/agent/app/httpserver/server_test.go

RickyRajinder · 2019-12-21T00:26:57Z

@yurishkuro Do I update the content/docs/next-release/faq in the documentation repo?

yurishkuro · 2019-12-21T00:55:16Z

Do I update the content/docs/next-release/faq in the documentation repo?

yes

RickyRajinder · 2019-12-21T01:39:29Z

@yurishkuro How would an import from the same repo resolve if it is pointing to the remote master that does not contain the branch's changes?

yurishkuro · 2019-12-21T01:51:02Z

Sorry, I don't follow.

RickyRajinder · 2019-12-21T01:59:15Z

@yurishkuro For example, when I import "github.com/jaegertracing/jaeger/cmd/agent/app/httpserver" the import points to the remote repo that has been downloaded in /vendor, which causes compilation errors. So my question is, how would I properly import the local package from my branch.

yurishkuro · 2019-12-21T02:44:54Z

There's something wrong with your Go setup then. Maybe you cloned this repo in the wrong location, it should be under $GOPATH/src/github.com/jaegertracing/jaeger/

codecov · 2019-12-21T05:47:55Z

Codecov Report

Merging #1990 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1990      +/-   ##
==========================================
+ Coverage   96.93%   96.95%   +0.02%     
==========================================
  Files         204      205       +1     
  Lines       10113    10114       +1     
==========================================
+ Hits         9803     9806       +3     
+ Misses        270      269       -1     
+ Partials       40       39       -1

Impacted Files	Coverage Δ
pkg/clientcfg/clientcfghttp/cfgmgr.go	`100% <100%> (ø)`
cmd/collector/app/http_handler.go	`100% <100%> (ø)`	⬆️
cmd/collector/app/zipkin/http_handler.go	`100% <100%> (ø)`	⬆️
pkg/clientcfg/clientcfghttp/handler.go	`100% <100%> (ø)`	⬆️
plugin/storage/badger/spanstore/reader.go	`96.79% <0%> (ø)`	⬆️
...lugin/sampling/strategystore/adaptive/processor.go	`100% <0%> (+0.79%)`	⬆️

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 7350001...85c144c. Read the comment docs.

RickyRajinder · 2019-12-21T07:25:25Z

@yurishkuro it appears cross-dock is failing and it is not clear from the logs as to why. Could this be related to #1455?

yurishkuro

This PR now creates a lot of new public APIs. I don't think it's necessary. What we need is to pull the HTTP handler from the agent into a shared location, e.g. /pkg/clientcfg/http where there's only a single public type HTTPHandler that has RegisterHander(mux) function. The actual HTTPServer should remain in the agent, and the collector has its own HTTPService (used for /api/traces endpoint). This will also mean the test infrastructure that you made public can remain private in that package.

cmd/agent/app/httpserver/server.go

cmd/all-in-one/main.go

RickyRajinder · 2019-12-22T22:05:06Z

This PR now creates a lot of new public APIs. I don't think it's necessary. What we need is to pull the HTTP handler from the agent into a shared location, e.g. /pkg/clientcfg/http where there's only a single public type HTTPHandler that has RegisterHander(mux) function. The actual HTTPServer should remain in the agent, and the collector has its own HTTPService (used for /api/traces endpoint). This will also mean the test infrastructure that you made public can remain private in that package.

@yurishkuro if RegisterHandler(mux) is shared, then doesn't that mean that the collector and agent will be sharing the same endpoints?

yurishkuro · 2019-12-22T23:06:14Z

Yes, since they are part of the implementation. And they can remain private

yurishkuro

I did the refactoring as a separate PR #1996, please rebase. It should limit the scope of this change to only registering the endpoints from collector.

cmd/collector/app/zipkin/http_handler.go

pkg/clientcfg/http/http_handler.go

RickyRajinder · 2019-12-31T23:07:39Z

I did the refactoring as a separate PR #1996, please rebase. It should limit the scope of this change to only registering the endpoints from collector.

So APIHandler in cmd/collector/app/http_handler.go should contain a clientcfghttp.HTTPHandler?

Return metricsFactory from test server init fix import structure fix unwanted change in agent server Add license on new file Signed-off-by: RickyRajinder <singh.sangh@gmail.com> Add tests for errors Refactor collector API handler to extend agent handler Remove unused fields Unexport fields in HttpHandler Update callers of NewAPIHandler Run go fmt Fix lint issues Fix spelling error in comment Refactor HTTP Handler to shared location Implement clientConfigManager interface for collector Run gofmt Resolve prior dependencies Rename HTTPHandler Update comments and fix failing test in agent Change mux version Add empty test Rebase master and register routes in APIHandler Remove empty test and rename handler variables Run fmt

yurishkuro

Looks good, almost there!

cmd/all-in-one/main.go

cmd/collector/app/http_handler.go

cmd/all-in-one/main.go

cmd/collector/main.go

Signed-off-by: Yuri Shkuro <ys@uber.com>

RickyRajinder requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners December 21, 2019 00:12

yurishkuro requested changes Dec 21, 2019

View reviewed changes

RickyRajinder force-pushed the expose-sampling-endpoint-in-collector branch 7 times, most recently from 12bf753 to 64afaff Compare December 21, 2019 05:47

RickyRajinder mentioned this pull request Dec 21, 2019

Update FAQ for jaegertracing/jaeger#1990 jaegertracing/documentation#341

Merged

yurishkuro reviewed Dec 21, 2019

View reviewed changes

cmd/agent/app/httpserver/server.go Outdated Show resolved Hide resolved

cmd/all-in-one/main.go Outdated Show resolved Hide resolved

RickyRajinder closed this Dec 22, 2019

RickyRajinder reopened this Dec 22, 2019

RickyRajinder force-pushed the expose-sampling-endpoint-in-collector branch from 64afaff to 2fdedfb Compare December 31, 2019 06:56

RickyRajinder force-pushed the expose-sampling-endpoint-in-collector branch 6 times, most recently from 16ba9f2 to d06b515 Compare December 31, 2019 08:38

yurishkuro reviewed Dec 31, 2019

View reviewed changes

cmd/collector/app/zipkin/http_handler.go Outdated Show resolved Hide resolved

pkg/clientcfg/http/http_handler.go Outdated Show resolved Hide resolved

RickyRajinder force-pushed the expose-sampling-endpoint-in-collector branch 2 times, most recently from 7604fa3 to a420344 Compare December 31, 2019 23:46

RickyRajinder force-pushed the expose-sampling-endpoint-in-collector branch from a420344 to 7350001 Compare January 1, 2020 00:25

yurishkuro reviewed Jan 1, 2020

View reviewed changes

yurishkuro and others added 7 commits January 1, 2020 12:11

Merge branch 'master' into expose-sampling-endpoint-in-collector

bb3c04a

Extend handler to support base path

26ddb61

Signed-off-by: Yuri Shkuro <ys@uber.com>

Implement ConfigManager helper

179fddb

Signed-off-by: Yuri Shkuro <ys@uber.com>

Use shared config mgr and mount new endpoints under /api

185bfa7

Signed-off-by: Yuri Shkuro <ys@uber.com>

Undo unnecessary changes

4aa1ac3

Signed-off-by: Yuri Shkuro <ys@uber.com>

Minor cleanup

cc1e5c7

Signed-off-by: Yuri Shkuro <ys@uber.com>

Add TODOs

8dff12a

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro approved these changes Jan 1, 2020

View reviewed changes

Fix test

85c144c

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro merged commit 1803e05 into jaegertracing:master Jan 1, 2020

objectiser mentioned this pull request Feb 28, 2020

Serve sampling strategies jaegertracing/jaeger-opentelemetry-collector#9

Closed

houstonheat mentioned this pull request Mar 24, 2021

Make the whole URL for remoteSampler / Service name configurable jaegertracing/jaeger-client-java#565

Closed

Sucharitha95 mentioned this pull request Nov 2, 2022

[Bug]: Sampling strategies not working as expected #3925

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add /api/sampling endpoint in collector #1990

Add /api/sampling endpoint in collector #1990

RickyRajinder commented Dec 21, 2019 •

edited by yurishkuro

Loading

RickyRajinder commented Dec 21, 2019

yurishkuro commented Dec 21, 2019

RickyRajinder commented Dec 21, 2019

yurishkuro commented Dec 21, 2019

RickyRajinder commented Dec 21, 2019

yurishkuro commented Dec 21, 2019

codecov bot commented Dec 21, 2019 •

edited

Loading

RickyRajinder commented Dec 21, 2019

yurishkuro left a comment

RickyRajinder commented Dec 22, 2019 •

edited

Loading

yurishkuro commented Dec 22, 2019

yurishkuro left a comment

RickyRajinder commented Dec 31, 2019

yurishkuro left a comment

Add /api/sampling endpoint in collector #1990

Add /api/sampling endpoint in collector #1990

Conversation

RickyRajinder commented Dec 21, 2019 • edited by yurishkuro Loading

Which problem is this PR solving?

Short description of the changes

RickyRajinder commented Dec 21, 2019

yurishkuro commented Dec 21, 2019

RickyRajinder commented Dec 21, 2019

yurishkuro commented Dec 21, 2019

RickyRajinder commented Dec 21, 2019

yurishkuro commented Dec 21, 2019

codecov bot commented Dec 21, 2019 • edited Loading

Codecov Report

RickyRajinder commented Dec 21, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

RickyRajinder commented Dec 22, 2019 • edited Loading

yurishkuro commented Dec 22, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

RickyRajinder commented Dec 31, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

RickyRajinder commented Dec 21, 2019 •

edited by yurishkuro

Loading

codecov bot commented Dec 21, 2019 •

edited

Loading

RickyRajinder commented Dec 22, 2019 •

edited

Loading