Add optional time window for GetTrace & SearchTrace of http_handler #6159

rim99 · 2024-11-04T13:17:33Z

Which problem is this PR solving?

Resolves #4150

Description of the changes

add optional time window when fetching trace by id

How was this change tested?

unittest

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: yarn lint and yarn test

cmd/anonymizer/app/query/query.go

storage/spanstore/interface.go

yurishkuro · 2024-11-21T13:57:17Z

please rebase

rim99 · 2024-11-21T14:51:44Z

@yurishkuro Updated bsaed on comments, please reivew this PR, thanks

Makefile.Protobuf.mk

plugin/storage/grpc/proto/storage.proto

yurishkuro · 2024-11-21T15:20:06Z

@rim99 this PR is too large, please break it into logical pieces. At minimum grpc storage changes could be a separate PR, e.g. changing the IDL. The anonymizer change can be separate.

yurishkuro · 2024-11-21T15:20:43Z

you have jaeger-ui submodule change, it's not related

rim99 · 2024-11-23T01:43:16Z

@rim99 this PR is too large, please break it into logical pieces. At minimum grpc storage changes could be a separate PR, e.g. changing the IDL. The anonymizer change can be separate.

Cool, I'll keep creating new PRs and rebasing until we're satisfied with the size of this PR.

And first PR is: #6242 . Other changes rely on this

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

rim99 · 2024-12-19T13:25:19Z

@yurishkuro Now this PR contains changes of http_handler only, please review

codecov · 2024-12-19T13:31:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.21%. Comparing base (86f2305) to head (3ba412f).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6159      +/-   ##
==========================================
- Coverage   96.22%   96.21%   -0.01%     
==========================================
  Files         361      362       +1     
  Lines       20621    20705      +84     
==========================================
+ Hits        19843    19922      +79     
- Misses        595      599       +4     
- Partials      183      184       +1

Flag	Coverage Δ
badger_v1	`9.04% <ø> (-0.01%)`	⬇️
badger_v2	`1.64% <ø> (-0.01%)`	⬇️
cassandra-4.x-v1-manual	`15.04% <ø> (+0.02%)`	⬆️
cassandra-4.x-v2-auto	`1.58% <ø> (-0.01%)`	⬇️
cassandra-4.x-v2-manual	`1.58% <ø> (-0.01%)`	⬇️
cassandra-5.x-v1-manual	`15.04% <ø> (+0.02%)`	⬆️
cassandra-5.x-v2-auto	`1.58% <ø> (-0.01%)`	⬇️
cassandra-5.x-v2-manual	`1.58% <ø> (-0.01%)`	⬇️
elasticsearch-6.x-v1	`18.76% <ø> (-0.02%)`	⬇️
elasticsearch-7.x-v1	`18.83% <ø> (-0.03%)`	⬇️
elasticsearch-8.x-v1	`19.00% <ø> (-0.02%)`	⬇️
elasticsearch-8.x-v2	`1.63% <ø> (-0.01%)`	⬇️
grpc_v1	`10.72% <ø> (+0.03%)`	⬆️
grpc_v2	`7.98% <ø> (+0.03%)`	⬆️
kafka-v1	`9.40% <ø> (+0.03%)`	⬆️
kafka-v2	`1.64% <ø> (-0.01%)`	⬇️
memory_v2	`1.64% <ø> (+<0.01%)`	⬆️
opensearch-1.x-v1	`18.88% <ø> (-0.03%)`	⬇️
opensearch-2.x-v1	`18.89% <ø> (-0.01%)`	⬇️
opensearch-2.x-v2	`1.64% <ø> (-0.01%)`	⬇️
tailsampling-processor	`0.47% <ø> (-0.01%)`	⬇️
unittests	`95.06% <100.00%> (-0.01%)`	⬇️

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.

cmd/query/app/http_handler.go

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

yurishkuro · 2024-12-20T14:27:05Z

cmd/query/app/http_handler.go

-		TraceID: traceID,
+		TraceID:   traceID,
+		StartTime: startTime,
+		EndTime:   endTime,


from the start of the function up to this point it's the same code as in getTrace. Please move it to parseGetTraceParameters helper

cmd/query/app/http_handler_test.go

yurishkuro · 2024-12-20T14:30:17Z

cmd/query/app/http_handler_test.go

+			var response structuredResponse
+			err := getJSON(ts.server.URL+`/api/traces?traceID=1&`+tc.query, &response)
+			require.Error(t, err)
+		})


how is this test different from the one on L426?

updated with multiple traceIds

Actually, URIs are different:

this is /api/traces?traceID=1, with a ?

the other is /api/traces/traceID=1, with a / in the same position

These are handled by different handler methods

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

yurishkuro · 2024-12-21T15:15:54Z

cmd/query/app/http_handler.go

+
+func (aH *APIHandler) parseGetTraceParameters(w http.ResponseWriter, r *http.Request) (spanstore.GetTraceParameters, bool) {
+	traceID, traceIdOk := aH.parseTraceID(w, r)
+	startTime, startTimeOk := aH.parseMicroseconds(w, r, startTimeParam)


you cannot defer checking the error like that because helper functions already called aH.handleError which writes to the client (you cannot do that more than once).

I see, updated as checking result immediately

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

rim99 mentioned this pull request Nov 4, 2024

Update proto for marshal issue jaegertracing/jaeger-idl#108

Closed

4 tasks

rim99 force-pushed the optinal-time-window-when-fetching-trace-by-id branch 3 times, most recently from cb5f24f to d00b4d3 Compare November 4, 2024 13:57

rim99 marked this pull request as ready for review November 4, 2024 14:07

rim99 requested a review from a team as a code owner November 4, 2024 14:07

rim99 requested a review from jkowall November 4, 2024 14:07

dosubot bot added area/storage enhancement labels Nov 4, 2024

rim99 changed the title ~~Add optinal time window in TraceGetParameters~~ Add optional time window in TraceGetParameters Nov 4, 2024

rim99 force-pushed the optinal-time-window-when-fetching-trace-by-id branch from a0bb411 to 74fd1e5 Compare November 10, 2024 11:55

yurishkuro reviewed Nov 17, 2024

View reviewed changes

cmd/anonymizer/app/query/query.go Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 17, 2024

View reviewed changes

storage/spanstore/interface.go Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 17, 2024

View reviewed changes

storage/spanstore/interface.go Outdated Show resolved Hide resolved

This was referenced Nov 21, 2024

[Draft] Mix test #6237

Closed

Auto-generate gogo annotations for api_v3 #6233

Merged

rim99 force-pushed the optinal-time-window-when-fetching-trace-by-id branch 2 times, most recently from 7c3ab0c to 3d939f2 Compare November 21, 2024 14:50

yurishkuro reviewed Nov 21, 2024

View reviewed changes

Makefile.Protobuf.mk Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 21, 2024

View reviewed changes

plugin/storage/grpc/proto/storage.proto Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 21, 2024

View reviewed changes

plugin/storage/grpc/proto/storage.proto Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 21, 2024

View reviewed changes

plugin/storage/grpc/proto/storage.proto Outdated Show resolved Hide resolved

rim99 mentioned this pull request Nov 23, 2024

[feat] Add time window for GetTrace in span store interface #6242

Merged

4 tasks

rim99 mentioned this pull request Nov 23, 2024

[storage_v1] Add time window to GetTraceRequest #6244

Merged

4 tasks

Add optional time window for GetTrace & SearchTrace

a35d464

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

rim99 force-pushed the optinal-time-window-when-fetching-trace-by-id branch from 295a9f3 to a35d464 Compare December 19, 2024 13:21

rim99 changed the title ~~Add optional time window in TraceGetParameters~~ Add optional time window for GetTrace & SearchTrace of http_handler Dec 19, 2024

yurishkuro reviewed Dec 19, 2024

View reviewed changes

cmd/query/app/http_handler.go Outdated Show resolved Hide resolved

yurishkuro reviewed Dec 19, 2024

View reviewed changes

cmd/query/app/http_handler.go Outdated Show resolved Hide resolved

handle parsing error

d8fc5f9

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

rim99 force-pushed the optinal-time-window-when-fetching-trace-by-id branch from 55ea627 to d8fc5f9 Compare December 20, 2024 14:13

add test for archive trace with time window

ae92cbd

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

yurishkuro added the changelog:bugfix-or-minor-feature label Dec 20, 2024

yurishkuro reviewed Dec 20, 2024

View reviewed changes

cmd/query/app/http_handler_test.go Show resolved Hide resolved

yurishkuro reviewed Dec 20, 2024

View reviewed changes

rim99 added 5 commits December 21, 2024 20:28

refactor impl

89f881b

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

update unittest

cc719ea

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

refactor

e845f96

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

fix

50ed388

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

update test

3c56050

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

rim99 force-pushed the optinal-time-window-when-fetching-trace-by-id branch from f9d81a5 to 3c56050 Compare December 21, 2024 13:32

yurishkuro reviewed Dec 21, 2024

View reviewed changes

check parse result right away

3ba412f

Signed-off-by: Zhang Xin <zhangxin@outlook.com>

yurishkuro approved these changes Dec 22, 2024

View reviewed changes

yurishkuro merged commit 9fc9d75 into jaegertracing:main Dec 22, 2024
54 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional time window for GetTrace & SearchTrace of http_handler #6159

Add optional time window for GetTrace & SearchTrace of http_handler #6159

rim99 commented Nov 4, 2024 •

edited

Loading

yurishkuro commented Nov 21, 2024

rim99 commented Nov 21, 2024

yurishkuro commented Nov 21, 2024 •

edited

Loading

yurishkuro commented Nov 21, 2024

rim99 commented Nov 23, 2024 •

edited

Loading

rim99 commented Dec 19, 2024

codecov bot commented Dec 19, 2024 •

edited

Loading

yurishkuro Dec 20, 2024

rim99 Dec 21, 2024

yurishkuro Dec 20, 2024

rim99 Dec 21, 2024 •

edited

Loading

rim99 Dec 21, 2024 •

edited

Loading

yurishkuro Dec 21, 2024

rim99 Dec 22, 2024

Add optional time window for GetTrace & SearchTrace of http_handler #6159

Add optional time window for GetTrace & SearchTrace of http_handler #6159

Conversation

rim99 commented Nov 4, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

yurishkuro commented Nov 21, 2024

rim99 commented Nov 21, 2024

yurishkuro commented Nov 21, 2024 • edited Loading

yurishkuro commented Nov 21, 2024

rim99 commented Nov 23, 2024 • edited Loading

rim99 commented Dec 19, 2024

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

yurishkuro Dec 20, 2024

Choose a reason for hiding this comment

rim99 Dec 21, 2024

Choose a reason for hiding this comment

yurishkuro Dec 20, 2024

Choose a reason for hiding this comment

rim99 Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

rim99 Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

yurishkuro Dec 21, 2024

Choose a reason for hiding this comment

rim99 Dec 22, 2024

Choose a reason for hiding this comment

rim99 commented Nov 4, 2024 •

edited

Loading

yurishkuro commented Nov 21, 2024 •

edited

Loading

rim99 commented Nov 23, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

rim99 Dec 21, 2024 •

edited

Loading

rim99 Dec 21, 2024 •

edited

Loading