Create a /deep-dependencies endpoint #6654

adityachopra29 · 2025-02-02T16:02:23Z

Which problem is this PR solving?

Resolves Create dummy /deep-dependencies endpoint #6606

Description of the changes

Create endpoint api/deep-dependencies for ddg.
Adds code for it to server

How was this change tested?

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: npm run lint and npm run test

codecov · 2025-02-02T16:10:25Z

Codecov Report

Attention: Patch coverage is 88.57143% with 4 lines in your changes missing coverage. Please review.

Project coverage is 96.01%. Comparing base (ce0f543) to head (e817951).

Files with missing lines	Patch %	Lines
...d/query/app/analytics/deep_dependencies_handler.go	87.87%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6654      +/-   ##
==========================================
- Coverage   96.06%   96.01%   -0.06%     
==========================================
  Files         364      365       +1     
  Lines       20696    20730      +34     
==========================================
+ Hits        19881    19903      +22     
- Misses        622      631       +9     
- Partials      193      196       +3

Flag	Coverage Δ
badger_v1	`9.76% <ø> (ø)`
badger_v2	`1.82% <ø> (ø)`
cassandra-4.x-v1-manual	`14.81% <ø> (ø)`
cassandra-4.x-v2-auto	`1.81% <ø> (ø)`
cassandra-4.x-v2-manual	`1.81% <ø> (ø)`
cassandra-5.x-v1-manual	`14.81% <ø> (ø)`
cassandra-5.x-v2-auto	`1.81% <ø> (ø)`
cassandra-5.x-v2-manual	`1.81% <ø> (ø)`
elasticsearch-6.x-v1	`19.15% <ø> (ø)`
elasticsearch-7.x-v1	`19.23% <ø> (ø)`
elasticsearch-8.x-v1	`19.40% <ø> (ø)`
elasticsearch-8.x-v2	`1.82% <ø> (ø)`
grpc_v1	`10.81% <ø> (ø)`
grpc_v2	`7.80% <ø> (ø)`
kafka-3.x-v1	`10.13% <ø> (ø)`
kafka-3.x-v2	`1.82% <ø> (ø)`
memory_v2	`1.82% <ø> (ø)`
opensearch-1.x-v1	`19.28% <ø> (ø)`
opensearch-2.x-v1	`?`
opensearch-2.x-v2	`1.82% <ø> (-0.12%)`	⬇️
tailsampling-processor	`0.48% <ø> (ø)`
unittests	`94.90% <88.57%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yurishkuro · 2025-02-02T21:13:04Z

Please see comments in #6608, the same applies here

adityachopra29 · 2025-02-03T19:14:17Z

@yurishkuro the /deep-dependencies url is working now, but there are some issues.

Actually jaeger-ui tries to find a focalNode (ie focalService and focalOperation), which is chosen from all the operations of the chosen service. Hence there doesnt seem to be a simple way of just hardcoding the deep-dependencies.
To show the working in the ui, I chose the hotrod example and a specific service (namely driver) to show the rendering.
I tried to set a default value for the focalNode as a temporary solution, but the problem remains that the focalService and focalOperation will have to exist for this to work.

REC-20250203235503.mp4

As per your suggestion, I changed the backend to /api/deep-dependencies. However, the frontend still looks for the backend data on /analytics/v1/dependencies . So we will need to raise a PR in the ui repo for the same.

yurishkuro · 2025-02-10T14:40:24Z

Sorry I don't follow your points about focal service. Isn't that a user choice after you return the data? Is that an input to the endpoint that returns the data? In either case the dummy endpoint can ignore that input and always return the same data, and to test it you can specify the focal service in the query URL (I assume).

Another thing I didn't understand is why did you have to run hotrod in your screencast? If the endpoint always returns dummy data, what was the point of sending data to Jaeger from HotROD?

yurishkuro · 2025-02-10T14:41:06Z

And yes, you will need to make a change to the UI to use a different URL for the data. We could make it configurable in the UI, to keep it backwards compatible.

adityachopra29 · 2025-02-11T18:43:25Z

In either case the dummy endpoint can ignore that input and always return the same data, and to test it you can specify the focal service in the query URL (I assume).

@yurishkuro
I was actually trying to find workarounds of not hardcoding the service and operation, and directly going to /deep-dependencies and chosing a service and operation, which was causing a problem. But I understand the need now.

If we go to http://localhost:16686/deep-dependencies?density=ppe&operation=opA&service=serviceA we get the required graph.

This was also the reason I was running the hotrod example, to mock services and operations.

Are there any other considerations? Otherwise, I will move on and write the tests for the same, and also raise a pr in the ui.

adityachopra29 · 2025-02-12T21:49:44Z

@yurishkuro I have added the tests and also raised the PR in the UI. Please have a look once.

cmd/query/app/analytics/http_gateway.go

yurishkuro · 2025-02-13T00:11:01Z

cmd/query/app/analytics/http_gateway.go

+	_ ...any, /* args */
+) *mux.Route {
+	var handler http.Handler = http.HandlerFunc(f)
+	handler = otelhttp.WithRouteTag(route, handler)


isn't tracing of the endpoint already taken care of at the top level?

But wouldn't that only trace the parent span? As I understand, the parent tracer will not go recursively into the children spans. Please clariy once.

you are not adding any child spans, you're adding another, duplicate layer of instrumentation. It's redundant.

cmd/query/app/analytics/http_gateway_test.go

cmd/query/app/server.go

Signed-off-by: adityachopra29 <[email protected]>

adityachopra29 · 2025-02-14T18:38:12Z

@yurishkuro I have made the changes. Please have a look.

yurishkuro · 2025-02-14T20:25:00Z

cmd/query/app/analytics/http_gateway.go

+// HTTPGateway exposes analytics HTTP endpoints.
+type HTTPGateway struct{}
+


not used

Suggested change

// HTTPGateway exposes analytics HTTP endpoints.

type HTTPGateway struct{}

yurishkuro · 2025-02-14T20:25:18Z

cmd/query/app/analytics/http_gateway.go

@@ -0,0 +1,90 @@
+// Copyright (c) 2025 The Jaeger Authors.


why is the file called http_gateway? It does nothing of the sort.

Yes. I had named it before when I had made the gateway struct. Will rename it to deep_dependencies_handler

cmd/query/app/analytics/package_test.go

yurishkuro · 2025-02-14T20:26:46Z

cmd/query/app/analytics/http_gateway.go

+
+	res, err := json.Marshal(dependencies)
+	if err != nil {
+		http.Error(w, "Failed to encode response", http.StatusNotImplemented)


StatusNotImplemented

it IS implemented.

Okay. I will use InternalServerError.

yurishkuro · 2025-02-14T20:27:40Z

cmd/query/app/analytics/http_gateway_test.go

+	if basePath != "" && basePath != "/" {
+		gw.router = gw.router.PathPrefix(basePath).Subrouter()
+	}


why is this relevant?

yurishkuro · 2025-02-14T20:29:07Z

cmd/query/app/analytics/http_gateway_test.go

+) *testGateway {
+	gw := setupHTTPGatewayNoServer(t, basePath)
+
+	httpServer := httptest.NewServer(gw.router)


there is no need to set up a server to test the RegisterRouter function. You can do it all with a mock http response recorder.

Signed-off-by: adityachopra29 <[email protected]>

yurishkuro · 2025-02-16T14:01:13Z

cmd/query/app/analytics/deep_dependencies_handler.go

+	router *mux.Router,
+	f func(http.ResponseWriter, *http.Request),
+	route string,
+	_ ...any, /* args */


what is this for?

yurishkuro · 2025-02-16T14:02:50Z

cmd/query/app/server.go

@@ -183,6 +183,8 @@ func initRouter(
 		Tracer:       telset.TracerProvider,
 	}).RegisterRoutes(r)

+	analytics.RegisterRoutes(r)


why isn't it called from apiHandler.RegisterRoutes(r)?

yurishkuro

please use the exact same structure/organization of code as in #6608

adityachopra29 requested a review from a team as a code owner February 2, 2025 16:02

adityachopra29 requested a review from pavolloffay February 2, 2025 16:02

dosubot bot added the enhancement label Feb 2, 2025

adityachopra29 force-pushed the add-deep-dependencies-endpoint branch from 8ff02e0 to 0e4d03e Compare February 3, 2025 18:16

adityachopra29 force-pushed the add-deep-dependencies-endpoint branch from 4c624a3 to 2389215 Compare February 12, 2025 20:57

adityachopra29 changed the title ~~add deep dependencies endpoint~~ Create a /deeo-dependencies endpoint Feb 12, 2025

adityachopra29 changed the title ~~Create a /deeo-dependencies endpoint~~ Create a /deep-dependencies endpoint Feb 12, 2025

adityachopra29 mentioned this pull request Feb 12, 2025

Change deep dependency endpoint jaegertracing/jaeger-ui#2661

Open

4 tasks