-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[remote-storage][v2] Add proto definition for GetTraces
rpc
#6730
[remote-storage][v2] Add proto definition for GetTraces
rpc
#6730
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
import "google/protobuf/timestamp.proto"; | ||
import "opentelemetry/proto/trace/v1/trace.proto"; | ||
|
||
option go_package = "storage_v2"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro do we want the package name to be this? or should it just be storage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package jaeger.storage.v2;
is the important thing. Go package doesn't matter much, but you would want it to be something that will result in a sensible location of the generated file.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6730 +/- ##
==========================================
- Coverage 96.06% 96.02% -0.04%
==========================================
Files 364 364
Lines 20696 20696
==========================================
- Hits 19881 19873 -8
- Misses 622 628 +6
- Partials 193 195 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
@@ -0,0 +1,47 @@ | |||
syntax = "proto3"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to have a folder for this file
- internal/storage/v2/grpc/proto/traces_storage.proto
+ internal/storage/v2/grpc/trace_storage.proto
also, we use singular tracestorage
in the package name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro is it fine for these files to sit with the storage implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok for now. We may want to move the proto file into jaeger-idl in the future.
some minor nits |
Signed-off-by: Mahad Zaryab <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Which problem is this PR solving?
Description of the changes
traces_storage.proto
that will contain the API for all methods pertaining to the traces storage.How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:npm run lint
andnpm run test