Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[refactor] Converge v2 API with v2 remote storage API #6784

Merged
merged 2 commits into from
Feb 28, 2025

Conversation

mahadzaryab1
Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 commented Feb 27, 2025

Which problem is this PR solving?

Description of the changes

  • This PR makes the following changes to converge the v2 storage API with the v2 remote storage API
    • NumTraces -> SearchDepth
    • Update documentation for TraceQueryParams

How was this change tested?

  • CI

Checklist

Signed-off-by: Mahad Zaryab <[email protected]>
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @mahadzaryab1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request focuses on converging the v2 storage API with the v2 remote storage API. This involves renaming several parameters and types to align with the naming conventions used in the v2 remote storage API. Specifically, NumTraces is renamed to SearchDepth, TraceQueryParams to TraceQueryParameters, GetTraceParams to GetTraceParameters, and OperationQueryParams to OperationQueryParameters. The changes are spread across multiple files in the cmd/jaeger, cmd/query, and internal/storage directories, ensuring consistency between the two APIs.

Highlights

  • API Convergence: The primary goal is to converge the v2 storage API with the v2 remote storage API to ensure consistency.
  • Parameter Renaming: Key parameters and types are renamed to align with the v2 remote storage API naming conventions.
  • Code Refactoring: The codebase is refactored to use the new parameter and type names, ensuring that all references are updated.

Changelog

Click here to see the changelog
  • cmd/jaeger/internal/exporters/storageexporter/exporter_test.go
    • Renamed GetTraceParams to GetTraceParameters in the test case.
  • cmd/jaeger/internal/integration/trace_reader.go
    • Renamed GetTraceParams to GetTraceParameters in the GetTraces function signature.
    • Renamed OperationQueryParams to OperationQueryParameters in the GetOperations function signature.
    • Renamed TraceQueryParams to TraceQueryParameters in the FindTraces function signature.
    • Renamed NumTraces to SearchDepth within the FindTraces function.
    • Renamed TraceQueryParams to TraceQueryParameters in the FindTraceIDs function signature.
  • cmd/query/app/apiv3/gateway_test.go
    • Renamed OperationQueryParams to OperationQueryParameters in runGatewayGetOperations.
    • Renamed GetTraceParams to GetTraceParameters in runGatewayGetTrace.
  • cmd/query/app/apiv3/grpc_handler.go
    • Renamed GetTraceParams to GetTraceParameters in the GetTrace function.
    • Renamed TraceQueryParams to TraceQueryParameters and NumTraces to SearchDepth in the internalFindTraces function.
    • Renamed OperationQueryParams to OperationQueryParameters in the GetOperations function.
  • cmd/query/app/apiv3/grpc_handler_test.go
    • Renamed GetTraceParams to GetTraceParameters in the TestGetTrace test cases.
    • Renamed GetTraceParams to GetTraceParameters in the TestGetTraceStorageError test.
  • cmd/query/app/apiv3/http_gateway.go
    • Renamed GetTraceParams to GetTraceParameters in the getTrace function.
    • Renamed TraceQueryParams to TraceQueryParameters and NumTraces to SearchDepth in the parseFindTracesQuery function.
    • Renamed OperationQueryParams to OperationQueryParameters in the getOperations function.
  • cmd/query/app/apiv3/http_gateway_test.go
    • Renamed GetTraceParams to GetTraceParameters in the TestHTTPGatewayGetTrace test cases.
    • Renamed TraceQueryParams to TraceQueryParameters and NumTraces to SearchDepth in the mockFindQueries function.
    • Renamed OperationQueryParams to OperationQueryParameters in the TestHTTPGatewayGetOperationsErrors test.
  • cmd/query/app/querysvc/v2/querysvc/service.go
    • Renamed GetTraceParams to GetTraceParameters and updated the TraceIDs field.
    • Renamed TraceQueryParams to TraceQueryParameters.
    • Renamed OperationQueryParams to OperationQueryParameters in the GetOperations function.
    • Renamed GetTraceParams to GetTraceParameters in the ArchiveTrace function.
  • cmd/query/app/querysvc/v2/querysvc/service_test.go
    • Renamed GetTraceParams to GetTraceParameters in the TestGetTraces_ErrorInReader test.
    • Renamed GetTraceParams to GetTraceParameters in the TestGetTraces_Success test.
    • Renamed GetTraceParams to GetTraceParameters in the TestGetTraces_WithRawTraces test.
    • Renamed GetTraceParams to GetTraceParameters in the TestGetTraces_TraceInArchiveStorage test.
    • Renamed OperationQueryParams to OperationQueryParameters in the TestGetOperations test.
    • Renamed TraceQueryParams to TraceQueryParameters in the TestFindTraces_Success test.
    • Renamed TraceQueryParams to TraceQueryParameters in the TestFindTraces_WithRawTraces_PerformsAdjustment test.
    • Renamed TraceQueryParams to TraceQueryParameters in the TestFindTraces_WithRawTraces_PerformsAggregation test.
    • Renamed GetTraceParams to GetTraceParameters in the TestArchiveTrace test.
  • internal/storage/integration/elasticsearch_test.go
    • Renamed GetTraceParams to GetTraceParameters in the testArchiveTrace function.
  • internal/storage/integration/integration.go
    • Renamed TraceQueryParams to TraceQueryParameters in the ToTraceQueryParams function.
    • Renamed NumTraces to SearchDepth in the ToTraceQueryParams function.
    • Renamed TraceQueryParams to TraceQueryParameters in the testGetServices function.
    • Renamed GetTraceParams to GetTraceParameters in the testGetLargeSpan function.
    • Renamed OperationQueryParams to OperationQueryParameters in the testGetOperations function.
    • Renamed GetTraceParams to GetTraceParameters in the testGetTrace function.
    • Renamed GetTraceParams to GetTraceParameters in the testGetTrace function's NotFound error subtest.
    • Renamed TraceQueryParams to TraceQueryParameters in the findTracesByQuery function.
  • internal/storage/v2/api/tracestore/mocks/Reader.go
    • Renamed TraceQueryParams to TraceQueryParameters in the FindTraceIDs mock function.
    • Renamed TraceQueryParams to TraceQueryParameters in the FindTraces mock function.
    • Renamed OperationQueryParams to OperationQueryParameters in the GetOperations mock function.
    • Renamed GetTraceParams to GetTraceParameters in the GetTraces mock function.
  • internal/storage/v2/api/tracestore/reader.go
    • Renamed GetTraceParams to GetTraceParameters in the Reader interface.
    • Renamed OperationQueryParams to OperationQueryParameters in the Reader interface.
    • Renamed TraceQueryParams to TraceQueryParameters in the Reader interface.
    • Renamed GetTraceParams to GetTraceParameters and updated its description.
    • Renamed TraceQueryParams to TraceQueryParameters and updated its description.
    • Renamed OperationQueryParams to OperationQueryParameters and updated its description.
    • Renamed TraceQueryParams to TraceQueryParameters in the ToSpanStoreQueryParameters function.
    • Renamed NumTraces to SearchDepth in the ToSpanStoreQueryParameters function.
  • internal/storage/v2/grpc/trace_storage.proto
    • Renamed GetTraceParams to GetTraceParameters and updated its description.
    • Renamed GetTraceParams to GetTraceParameters in the GetTracesRequest message.
  • internal/storage/v2/v1adapter/spanreader.go
    • Renamed GetTraceParams to GetTraceParameters in the GetTrace function.
    • Renamed OperationQueryParams to OperationQueryParameters in the GetOperations function.
    • Renamed TraceQueryParams to TraceQueryParameters and NumTraces to SearchDepth in the FindTraces function.
    • Renamed TraceQueryParams to TraceQueryParameters and NumTraces to SearchDepth in the FindTraceIDs function.
  • internal/storage/v2/v1adapter/spanreader_test.go
    • Renamed GetTraceParams to GetTraceParameters in the TestSpanReader_GetTrace test cases.
    • Renamed OperationQueryParams to OperationQueryParameters in the TestSpanReader_GetOperations test cases.
    • Renamed TraceQueryParams to TraceQueryParameters in the TestSpanReader_FindTraces test cases.
    • Renamed TraceQueryParams to TraceQueryParameters in the TestSpanReader_FindTraceIDs test cases.
  • internal/storage/v2/v1adapter/tracereader.go
    • Renamed GetTraceParams to GetTraceParameters in the GetTraces function.
    • Renamed OperationQueryParams to OperationQueryParameters in the GetOperations function.
    • Renamed TraceQueryParams to TraceQueryParameters in the FindTraces function.
    • Renamed TraceQueryParams to TraceQueryParameters in the FindTraceIDs function.
  • internal/storage/v2/v1adapter/tracereader_test.go
    • Renamed GetTraceParams to GetTraceParameters in the TestTraceReader_GetTracesDelegatesSuccessResponse test.
    • Renamed GetTraceParams to GetTraceParameters in the TestTraceReader_GetTracesErrorResponse test.
    • Renamed GetTraceParams to GetTraceParameters in the TestTraceReader_GetTracesEarlyStop test.
    • Renamed OperationQueryParams to OperationQueryParameters in the TestTraceReader_GetOperationsDelegatesResponse test.
    • Renamed TraceQueryParams to TraceQueryParameters in the TestTraceReader_FindTracesDelegatesSuccessResponse test.
    • Renamed TraceQueryParams to TraceQueryParameters in the TestTraceReader_FindTracesEdgeCases test.
    • Renamed TraceQueryParams to TraceQueryParameters in the TestTraceReader_FindTracesEarlyStop test.
    • Renamed TraceQueryParams to TraceQueryParameters in the TestTraceReader_FindTraceIDsDelegatesResponse test.
  • proto-gen/storage/v2/trace_storage.pb.go
    • Renamed GetTraceParams to GetTraceParameters and updated its description.
    • Renamed GetTraceParams to GetTraceParameters in the GetTracesRequest message.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


Trivia time!

What is the purpose of Protocol Buffers (protobuf) used for defining the gRPC API?

Click here for the answer
Protocol Buffers (protobuf) are a language-neutral, platform-neutral extensible mechanism for serializing structured data. They are used to define the structure of data exchanged between services in gRPC, ensuring efficient and reliable communication.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request focuses on converging the v2 storage API with the v2 remote storage API by renaming several parameters and types. The changes seem straightforward and well-tested, as indicated by the CI passing and the inclusion of unit tests. The renaming aligns the codebase with the new API, improving consistency.

Summary of Findings

Merge Readiness

The changes in this pull request appear to be well-implemented and tested. The renaming of parameters and types contributes to a more consistent and understandable API. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. Given the nature of the changes and the included tests, the pull request seems ready for merging, assuming that the changes do not break any dependent code.

@mahadzaryab1 mahadzaryab1 requested a review from Copilot February 27, 2025 22:50

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR refactors the v2 storage API to converge its naming conventions with the v2 remote storage API. Key changes include renaming types and parameters such as:

  • "GetTraceParams" to "GetTraceParameters"
  • "TraceQueryParams" to "TraceQueryParameters"
  • "OperationQueryParams" to "OperationQueryParameters"
  • Replacing the "NumTraces" field with "SearchDepth" in related structs

Reviewed Changes

File Description
internal/storage/v2/api/tracestore/reader.go Renamed types and updated field "NumTraces" to "SearchDepth" in the trace query parameters and mappings
cmd/query/app/apiv3/http_gateway_test.go Updated test expectations to use the new parameter type names
internal/storage/v2/v1adapter/tracereader_test.go Updated tests to reflect the new type names and field renaming
proto-gen/storage/v2/trace_storage.pb.go Adjusted generated types and registration for the renamed proto messages
... Other files (mocks, integration tests, handler tests, etc.) have been updated consistently

Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

internal/storage/v2/api/tracestore/reader.go:116

  • [nitpick] The conversion in ToSpanStoreQueryParameters maps 'SearchDepth' to the target field 'NumTraces'. This naming difference can be confusing; consider adding a comment to clarify why the field is named differently in the destination struct or evaluate aligning the field names for consistency.
NumTraces:     t.SearchDepth,
Copy link

codecov bot commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.03%. Comparing base (5123ffd) to head (8438b69).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6784      +/-   ##
==========================================
+ Coverage   96.02%   96.03%   +0.01%     
==========================================
  Files         364      364              
  Lines       20690    20690              
==========================================
+ Hits        19867    19870       +3     
+ Misses        628      626       -2     
+ Partials      195      194       -1     
Flag Coverage Δ
badger_v1 9.81% <33.33%> (ø)
badger_v2 1.89% <0.00%> (ø)
cassandra-4.x-v1-manual 14.86% <33.33%> (ø)
cassandra-4.x-v2-auto 1.88% <0.00%> (ø)
cassandra-4.x-v2-manual 1.88% <0.00%> (ø)
cassandra-5.x-v1-manual 14.86% <33.33%> (ø)
cassandra-5.x-v2-auto 1.88% <0.00%> (ø)
cassandra-5.x-v2-manual 1.88% <0.00%> (ø)
elasticsearch-6.x-v1 19.19% <33.33%> (ø)
elasticsearch-7.x-v1 19.27% <33.33%> (ø)
elasticsearch-8.x-v1 19.44% <33.33%> (ø)
elasticsearch-8.x-v2 1.89% <0.00%> (ø)
grpc_v1 10.86% <33.33%> (ø)
grpc_v2 7.86% <0.00%> (ø)
kafka-3.x-v1 10.11% <0.00%> (ø)
kafka-3.x-v2 1.89% <0.00%> (ø)
memory_v2 1.89% <0.00%> (ø)
opensearch-1.x-v1 19.32% <33.33%> (ø)
opensearch-2.x-v1 19.32% <33.33%> (ø)
opensearch-2.x-v2 1.89% <0.00%> (ø)
tailsampling-processor 0.48% <0.00%> (ø)
unittests 94.92% <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.

@mahadzaryab1 mahadzaryab1 added this pull request to the merge queue Feb 28, 2025
Merged via the queue into jaegertracing:main with commit 07430f7 Feb 28, 2025
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants