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

Optionally supply tenancy when reporting spans #3742

Closed
wants to merge 2 commits into from

Conversation

esnible
Copy link
Contributor

@esnible esnible commented Jun 8, 2022

Signed-off-by: Ed Snible snible@us.ibm.com

With #3688 we introduced --multi_tenancy.enabled=true which enables the collector to reject spans with missing or invalid tenant header.

This PR restores correct span reporting when tenancy is enabled. Without this change, when a multi-tenant Jaeger gets a (currently untenanted) query, it reports spans related to handling the query without a tenant. Those (usually) self spans are rejected.

Some of the work in this PR was previously done in the draft PR for query tenancy, #3735 . That PR was getting big so the reporting stuff will be removed and handled in this PR.

Signed-off-by: Ed Snible <snible@us.ibm.com>
@esnible esnible requested a review from a team as a code owner June 8, 2022 19:29
@esnible esnible requested a review from pavolloffay June 8, 2022 19:29
Signed-off-by: Ed Snible <snible@us.ibm.com>
@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #3742 (f30c9f6) into main (1ed6491) will decrease coverage by 0.01%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##             main    #3742      +/-   ##
==========================================
- Coverage   97.49%   97.48%   -0.02%     
==========================================
  Files         271      271              
  Lines       16004    16017      +13     
==========================================
+ Hits        15603    15614      +11     
- Misses        317      319       +2     
  Partials       84       84              
Impacted Files Coverage Δ
cmd/agent/app/reporter/grpc/builder.go 95.16% <ø> (ø)
cmd/agent/app/proxy_builders.go 57.14% <40.00%> (-42.86%) ⬇️
cmd/agent/app/reporter/grpc/collector_proxy.go 100.00% <100.00%> (ø)
cmd/agent/app/reporter/grpc/flags.go 100.00% <100.00%> (ø)
cmd/agent/app/reporter/grpc/reporter.go 100.00% <100.00%> (ø)
pkg/config/tlscfg/cert_watcher.go 92.55% <0.00%> (-2.13%) ⬇️
plugin/storage/badger/spanstore/reader.go 96.20% <0.00%> (+0.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ed6491...f30c9f6. Read the comment docs.

return grpc.NewCollectorProxy(builder, opts.AgentTags, opts.Metrics, opts.Logger)
md := metadata.New(map[string]string{})
if builder.CollectorTenancyHeader != "" {
md = metadata.New(map[string]string{builder.CollectorTenancyHeader: builder.CollectorTenant})
Copy link
Member

Choose a reason for hiding this comment

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

With this change every span received by the Agent will be tagged with a single tenant when forwarded to Collector. While this is a viable deployment model in some situations, I don't think it's ultimately the right one, especially when the Agent runs as a host agent receiving data from potentially multiple co-located applications (which could be different tenants).

For example, this makes it impossible to test multi-tenancy with all-in-one if the other tenants are also going through the agent.

My preference would be for the agent to extract the tenant directly from the spans it receives, and the current solution could be an opt-in extension to either override the tenant or inject it when empty (for cases when agent is used as a sidecar).

@esnible
Copy link
Contributor Author

esnible commented Jun 9, 2022

Closing in favor of #3750

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants