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.

Sign up for GitHub

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

Jump to bottom

[grpc storage]: Propagate tenant to grpc backend #6030

Merged
merged 3 commits into from
Oct 3, 2024
Merged

[grpc storage]: Propagate tenant to grpc backend #6030

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
090962d
storage: propagate tenant to grpc backend
frzifus Sep 30, 2024
37fb0e1
Revert "storage: propagate tenant to grpc backend"
frzifus Oct 1, 2024
20a31ba
fix: translate tenancy config from v1 to v2
frzifus Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions plugin/storage/grpc/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ func DefaultConfigV2() ConfigV2 {

func (c *Configuration) TranslateToConfigV2() *ConfigV2 {
return &ConfigV2{
Tenancy: c.TenancyOpts,
Copy link
Member

Choose a reason for hiding this comment

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

@mahadzaryab1 I think this is another example of divergence between v1/v2 configs - this is why my preference is always NOT to have separate configs, so that omissions like this won't happen. Do you want to take a look at that as part of #5229?

Copy link
Member

Choose a reason for hiding this comment

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

@frzifus what kind of unit or e2e test could we have to catch this bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was going to add a unittest for this translate config function. But when I started to re-type this 4 line logic, I thought it doesnt make much sense.

An e2e test that runs jaeger-query with the SPAN_STORAGE_TYPE=grpc storage and the --multi-tenancy.header=x-scope-orgid flag and checks the metadata of the outgoing gRPC request would be perfect to avoid that issue in the future.
Similar to this: #6030 (comment)

@yurishkuro Would it be ok, creating an issue to keep track and submit something afterwards?

Copy link
Collaborator

Choose a reason for hiding this comment

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

will do @yurishkuro

Copy link
Member

Choose a reason for hiding this comment

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

An e2e test that runs jaeger-query with the SPAN_STORAGE_TYPE=grpc storage and the --multi-tenancy.header=x-scope-orgid flag and checks the metadata of the outgoing gRPC request would be perfect to avoid that issue in the future.

We have a grpc storage e2e test today. We can add a test there that will run the storage with tenancy enabled and store a span with tenant1 then try to retrieve it with tenant1 (found) and tenant2 (not found). The memory storage used in that e2e test already supports separation of data by tenant.

Copy link
Member

Choose a reason for hiding this comment

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

It would be even better to do this with jaeger-v2 e2e tests, but one thing that I don't know about is how we can thread the tenant ID through the OTEL Collector pipeline, from OTLP receiver to jaeger-storage-extension.

ClientConfig: configgrpc.ClientConfig{
Endpoint: c.RemoteServerAddr,
TLSSetting: c.RemoteTLS.ToOtelClientConfig(),
Expand Down
Loading