[WIP] Add Adaptive Sampling Support for gRPC Remote Storage #6308
Conversation
Signed-off-by: Alok Kumar Singh <[email protected]>
|
Creating a draft PR to get reviews if I am on the right track or probably missing something. Thanks |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6308 +/- ##
==========================================
+ Coverage 48.78% 49.06% +0.27%
==========================================
Files 181 183 +2
Lines 11111 11352 +241
==========================================
+ Hits 5421 5570 +149
- Misses 5241 5317 +76
- Partials 449 465 +16
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: Alok Kumar Singh <[email protected]>
|
you are on the right track, but before you continue, what is your plan for e2e testing? |
Signed-off-by: Alok Kumar Singh <[email protected]>
I was thinking of using one of the already created backend implementation with a gRPC connection. |
|
ok. Maybe start with that. |
Signed-off-by: Alok Kumar Singh <[email protected]>
Signed-off-by: Alok Kumar Singh <[email protected]>
Signed-off-by: Alok Kumar Singh <[email protected]>
akstron
changed the title
cmd/remote-storage/app/server.go
Outdated
| @@ -67,12 +68,17 @@ func createGRPCHandler(f storage.BaseFactory, logger *zap.Logger) (*shared.GRPCH | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| samplingStore, err := samplingStoreFactory.CreateSamplingStore(1) | |||
There was a problem hiding this comment.
for readability, define named constant instead of passing unnamed 1
| @@ -374,6 +377,7 @@ func TestServerHandlesPortZero(t *testing.T) { | |||
| storageMocks.factory, | |||
| tenancy.NewManager(&tenancy.Options{}), | |||
| telset, | |||
| nil, | |||
There was a problem hiding this comment.
please add a test where something real is passed
cmd/remote-storage/main.go
Outdated
| @@ -77,7 +77,7 @@ func main() { | |||
| tm := tenancy.NewManager(&opts.Tenancy) | |||
| telset := baseTelset // copy | |||
| telset.Metrics = metricsFactory | |||
| server, err := app.NewServer(opts, storageFactory, tm, telset) | |||
| server, err := app.NewServer(opts, storageFactory, tm, telset, nil) | |||
There was a problem hiding this comment.
how will it work if you pass nil?
| @@ -148,6 +148,63 @@ message FindTraceIDsResponse { | |||
| ]; | |||
| } | |||
|
|
|||
| message Throughput { | |||
There was a problem hiding this comment.
Schema should be documented. Wherever you are copying this from, we have comments explaining all types.
| @@ -112,6 +112,7 @@ func TestNewGRPCClient(t *testing.T) { | |||
| assert.Implements(t, (*storage_v1.PluginCapabilitiesClient)(nil), client.capabilitiesClient) | |||
| assert.Implements(t, (*storage_v1.DependenciesReaderPluginClient)(nil), client.depsReaderClient) | |||
| assert.Implements(t, (*storage_v1.StreamingSpanWriterPluginClient)(nil), client.streamWriterClient) | |||
| assert.Implements(t, (*storage_v1.SamplingStorePluginClient)(nil), client.samplingStoreClient) | |||
There was a problem hiding this comment.
no unit tests for new functions?
Signed-off-by: Alok Kumar Singh <[email protected]>
Signed-off-by: Alok Kumar Singh <[email protected]>
akstron
changed the title
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test