[refactor][query] Propagate `RawTraces` flag to query service by mahadzaryab1 · Pull Request #6438 · jaegertracing/jaeger

mahadzaryab1 · 2024-12-28T16:04:35Z

Which problem is this PR solving?

Towards Enable usage of adjusters in api_v2 and api_v3 handlers #6417

Description of the changes

This PR defines GetTraceParamaters and TraceQueryParameters inside package querysvc that are currently just wrappers around their package spanstore counterparts.
This is done so that additional parameters can be passed into the query service, like the RawTraces flag, without having to extend the parameters that are passed into the storage implementations.

How was this change tested?

CI

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

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

codecov · 2024-12-28T16:11:50Z

Codecov Report

Attention: Patch coverage is 98.13084% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.26%. Comparing base (e7a0205) to head (aa57f7d).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/query/app/apiv3/http_gateway.go	91.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6438      +/-   ##
==========================================
+ Coverage   96.23%   96.26%   +0.02%     
==========================================
  Files         368      368              
  Lines       20978    21028      +50     
==========================================
+ Hits        20189    20242      +53     
+ Misses        604      602       -2     
+ Partials      185      184       -1

Flag	Coverage Δ
badger_v1	`10.56% <0.00%> (ø)`
badger_v2	`2.42% <0.00%> (ø)`
cassandra-4.x-v1-manual	`16.45% <0.00%> (ø)`
cassandra-4.x-v2-auto	`2.36% <0.00%> (ø)`
cassandra-4.x-v2-manual	`2.36% <0.00%> (ø)`
cassandra-5.x-v1-manual	`16.45% <0.00%> (ø)`
cassandra-5.x-v2-auto	`2.36% <0.00%> (ø)`
cassandra-5.x-v2-manual	`2.36% <0.00%> (ø)`
elasticsearch-6.x-v1	`20.20% <0.00%> (+<0.01%)`	⬆️
elasticsearch-7.x-v1	`20.27% <0.00%> (+<0.01%)`	⬆️
elasticsearch-8.x-v1	`20.43% <0.00%> (+<0.01%)`	⬆️
elasticsearch-8.x-v2	`2.42% <0.00%> (+<0.01%)`	⬆️
grpc_v1	`12.23% <0.00%> (+<0.01%)`	⬆️
grpc_v2	`8.82% <0.00%> (+<0.01%)`	⬆️
kafka-3.x-v1	`10.40% <0.00%> (ø)`
kafka-3.x-v2	`2.42% <0.00%> (ø)`
memory_v2	`2.42% <0.00%> (ø)`
opensearch-1.x-v1	`20.32% <0.00%> (ø)`
opensearch-2.x-v1	`20.32% <0.00%> (ø)`
opensearch-2.x-v2	`2.42% <0.00%> (+<0.01%)`	⬆️
tailsampling-processor	`0.39% <0.00%> (ø)`
unittests	`95.12% <98.13%> (+0.02%)`	⬆️

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.

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

cmd/query/app/grpc_handler.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro · 2024-12-28T17:33:38Z

cmd/query/app/apiv3/http_gateway_test.go

+		{
+			name:          "TestGetTraceWithInvalidRawTraces",
+			requestUrl:    "/api/v3/traces/123?query.raw_traces=foobar",
+			expectedError: "malformed parameter query.raw_traces",


curious - why above error messages did not include query. prefix for param name?

Those parameters don't have the query prefix - see https://github.com/jaegertracing/jaeger/blob/main/cmd/query/app/apiv3/http_gateway.go#L30-L32

It's confusing to me why that's the case (this can be seen in the swagger file). But, regardless, according to swagger.json, when the url is traces/?... it's interpreted as FindTraces and all params have the query. prefix, but if the url is traces/{traceID}?... then it's the GetTrace method and the params don't have the prefix. Meaning that the above example is an invalid query.

Got it! Thanks for the explanation - I fixed this now to have two constants.

cmd/query/app/grpc_handler.go

cmd/query/app/http_handler.go

yurishkuro · 2024-12-28T17:40:23Z

cmd/query/app/http_handler_test.go

+	var response structuredResponse
+	err := getJSON(ts.server.URL+`/api/traces/`+mockTraceID.String()+`?raw=true`, &response)
+	require.NoError(t, err)
+	assert.Empty(t, response.Errors)


the more accurate test would be to use a mock trace that has a known issue that adjusters would fix (e.g. same ID for client/server spans), run the query twice with raw=true|false, and observe the difference in the output.

@yurishkuro Agreed. But this PR doesn't actually implement the differentiating logic but rather just passes the flag down. That will happen in #6423.

yurishkuro · 2024-12-28T17:44:05Z

cmd/query/app/http_handler.go

+	return false, true
+}
+
+func (aH *APIHandler) parseGetTraceParameters(w http.ResponseWriter, r *http.Request) (querysvc.GetTraceParameters, bool) {


Side note: this function is poorly designed. A better design is query_parser.parseTraceQueryParams, which simply returns errors. The aH.handleError should really be used only rarely, directly in the handler function.

do we want to address this in a future PR?

it's one of these nice to have clean-ups, I'd rather have it as good-first-issue for someone other than you spending time on it.

cmd/query/app/querysvc/query_service.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro · 2024-12-28T23:20:43Z

cmd/query/app/querysvc/query_service.go


 // ArchiveTrace is the queryService utility to archive traces.
-func (qs QueryService) ArchiveTrace(ctx context.Context, query spanstore.GetTraceParameters) error {
+func (qs QueryService) ArchiveTrace(ctx context.Context, query GetTraceParameters) error {


should this remain spanstore.GetTraceParameters? Or ArchiveTraceParameters, but Get with raw doesn't apply here.

@yurishkuro Thanks for catching. This was an oversight - I reverted it back for now. We can always create a separate type here if we need to in the future.

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 · 2024-12-28T23:33:22Z

@yurishkuro I think there are two lines that are missing code coverage at the moment in the http_gateway.go. It should be taken care of in the next PR as I will add tests to all the handlers to ensure that the RawTraces flag is being respected but let me know if you prefer adding some boilerplate tests in this PR to get codecov up to 100%.

mahadzaryab1 added 4 commits December 28, 2024 10:47

Define GetTraceParameters in the Query Service

8b235f1

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Existing Usages

c91b5c8

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Define TraceQueryParameters in the Query Service

1029bb5

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Existing Usages

3191737

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 added the changelog:bugfix-or-minor-feature label Dec 28, 2024

mahadzaryab1 requested a review from yurishkuro December 28, 2024 16:04

mahadzaryab1 requested a review from a team as a code owner December 28, 2024 16:04

mahadzaryab1 mentioned this pull request Dec 28, 2024

[query] Enable trace adjusters in api_v2 and api_v3 handlers #6423

Merged

4 tasks

Fix Typo

e630497

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro reviewed Dec 28, 2024

View reviewed changes

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

mahadzaryab1 added 5 commits December 28, 2024 12:15

Accept RawTraces Flag In Query Parameters

59beaef

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Update Query Parser To Pass Raw Traces

e94f8af

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Update HTTP Handler To Pass Raw Traces

513663a

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Update HTTP Gateway To Pass Raw Traces

5f8a21a

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Update GRPC Handlers To Pass Raw Traces

00ec4d9

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 changed the title ~~[refactor][query] Define GetTraceParameters and TraceQueryParameters in query service~~ [refactor][query] Propagate RawTraces flag to query service Dec 28, 2024

Fix Linting Error

72c6670

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro reviewed Dec 28, 2024

View reviewed changes

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

yurishkuro reviewed Dec 28, 2024

View reviewed changes

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

yurishkuro reviewed Dec 28, 2024

View reviewed changes

cmd/query/app/querysvc/query_service.go Outdated Show resolved Hide resolved

mahadzaryab1 added 5 commits December 28, 2024 17:57

Move Structs To Top

3417d76

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Pass Raw Flag To Search Function

b1b88a4

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Parameter Mismatch

16dbe8d

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Add TODO Comment To Test

2c2306a

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Add Test For Error Case in Find Traces

8297e38

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro reviewed Dec 28, 2024

View reviewed changes

Revert ArchiveTrace Parameter Back

aa57f7d

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro approved these changes Dec 28, 2024

View reviewed changes

yurishkuro merged commit a6616fb into jaegertracing:main Dec 28, 2024

mahadzaryab1 deleted the query-service-params branch January 6, 2025 00:42

Conversation

mahadzaryab1 commented Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Uh oh!

codecov bot commented Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahadzaryab1 commented Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mahadzaryab1 commented Dec 28, 2024 •

edited

Loading

codecov bot commented Dec 28, 2024 •

edited

Loading

mahadzaryab1 commented Dec 28, 2024 •

edited

Loading