[jaeger-v2] add gRPC integration e2e test mode #5322
Conversation
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
|
@yurishkuro There's an issue that translating Jaeger proto to OTLP format changes the |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5322 +/- ##
==========================================
- Coverage 95.25% 95.17% -0.09%
==========================================
Files 340 343 +3
Lines 16677 16779 +102
==========================================
+ Hits 15886 15969 +83
- Misses 601 610 +9
- Partials 190 200 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
| ) | ||
|
|
||
| type GRPCServer struct { | ||
| errChan chan error |
There was a problem hiding this comment.
Is this actually needed? If it is, I would prefer to use a callback function, not a channel, because channels like this tend to cause deadlocks when nothing is reading them, while the callback achieves the same result of communicating the status without the risk of blocking (and the implementation of the callback can still use channel if it wants to, but that channel will be more local and less prone to deadlocks)
There was a problem hiding this comment.
I see, I have updated this to pass the error through the struct field
| require.NoError(t, err) | ||
| require.NoError(t, s.factory.Close()) | ||
| if s.server != nil { | ||
| require.NoError(t, s.server.Close()) |
There was a problem hiding this comment.
Would it make sense to release the server here (s.server = nil)?
There was a problem hiding this comment.
Hmm.. actually the server (GRPCServer) here is a declaration if the current test suite uses remote storage or a plugin. If it uses remote storage then it closes and starts the server again.
But the server inside GPRCServer struct is the gRPC server that handles requests, and I'll add to release the server there.
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
| return err | ||
| } | ||
|
|
||
| func (r *spanReader) GetTrace(ctx context.Context, traceID model.TraceID) (*model.Trace, error) { |
There was a problem hiding this comment.
a bit sad that you had to implement all of that. This is a takeway for V2 storage - perhaps it's possible to have remote storage API to be the same as query service API. Then we could've used plugin/storage/grpc/shared/client
There was a problem hiding this comment.
Okay, I'll create a new issue for this task.
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
yurishkuro
added
the
changelog:test
Signed-off-by: James Ryans <[email protected]>
| "github.com/jaegertracing/jaeger/ports" | ||
| ) | ||
|
|
||
| type GRPCServer struct { |
There was a problem hiding this comment.
What is the business function of this class? Calling it grpc server doesn't tell me anything. Is it not RemoteMemoryStorage?
There was a problem hiding this comment.
Also, what is the difference between this server and cmd/remote-storage/app/server.go? Could we reuse that one?
There was a problem hiding this comment.
Actually yes, we could reuse cmd/remote-storage/app/server.go. This server is just an extraction of the gRPCServer from existing grpc_test.go so I can reuse this code for the new e2e grpc_test, but I'll change this to reuse the remote-storage server.
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| cc, err := grpc.DialContext(ctx, ports.PortToHostPort(ports.QueryGRPC), opts...) |
There was a problem hiding this comment.
Call NewClient, dial was deprecated
| } | ||
| } | ||
|
|
||
| func (r *spanReader) Start() error { |
There was a problem hiding this comment.
I suggest merging this into create function. What's the benefit of separating Start?
There was a problem hiding this comment.
Okay. I've seen a lot of structs that separate the creation and start functions so I thought it was cleaner that way. I'll refactor this.
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
|
@james-ryans another possible way to unblock from waiting on OTEL contrib is to tweak the tests to not use binary tags (controlled by a flag, with a new issue to fix it once OTEL fix is released) |
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
|
|
||
| // TODO: remove this after upstream issue in OTEL jaeger translator is fixed | ||
| // Skip testing trace binary tags, logs, and process | ||
| SkipBinaryAttrs bool |
There was a problem hiding this comment.
please book a ticket referring to this commit
|
|
||
| func (s *RemoteMemoryStorage) Close() error { | ||
| if err := s.server.Close(); err != nil { | ||
| return err |
There was a problem hiding this comment.
nit: this potentially leaves storageFactory not closed. The pattern we use for multi-close is
return errors.Join(
b.reporter.Close(),
b.tlsCloser.Close(),
b.GetConn().Close(),
)
| tm := tenancy.NewManager(&opts.Tenancy) | ||
| storageFactory, err := storage.NewFactory(storage.FactoryConfigFromEnvAndCLI(os.Args, os.Stderr)) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
wdyt about taking t.Testing arg and just calling require.NoError? This will eliminate the uncovered lines.
There was a problem hiding this comment.
Yes, this is the easiest approach. Will do
|
|
||
| func TestIndexCleaner_doNotFailOnFullStorage(t *testing.T) { | ||
| skipUnlessEnv(t, "elasticsearch", "opensearch") | ||
| SkipUnlessEnv(t, "elasticsearch", "opensearch") |
There was a problem hiding this comment.
this little change is a good candidate for a separate PR. It would significantly reduce the number of files in this PR, making it more focused.
There was a problem hiding this comment.
I see, I'll do that
Signed-off-by: James Ryans <[email protected]>
## Which problem is this PR solving? - PR #5322 temporarily added a SkipBinaryAttrs flag to avoid checking span's tags with a binary type since the OTEL Jaeger translator has a bug that converts binary tags into string tags. Since it has been fixed, we will delete this flag. ## Description of the changes - Delete the SkipBinaryAttrs flag from StorageIntegration. ## How was this change tested? - Tested locally by running all the e2e storage tests. e.g. `STORAGE=grpc SPAN_STORAGE_TYPE=memory make jaeger-v2-storage-integration-test` ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [ ] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: James Ryans <[email protected]>
2 participants
Which problem is this PR solving?
Description of the changes
StorageIntegrationto test the jaeger-v2 OTel Collector and gRPC storage backend with the provided config file atcmd/jaeger/grpc_config.yaml.How was this change tested?
STORAGE=grpc SPAN_STORAGE_TYPE=memory make jaeger-v2-storage-integration-testChecklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test