feat: adding prefix_prefer_source option #62
Conversation
processor.go
Outdated
| if p.cfg.Tenant.Prefix != "" { | ||
| tenant = p.cfg.Tenant.Prefix + tenant | ||
| if tenantPrefix != "" { | ||
| tenant = tenantPrefix + tenant |
There was a problem hiding this comment.
Why do we need to pass the prefix that deep? I think it's enough just to pass it to dispatch() and there prefix the tenants that are passed to p.send() with it. And if p.cfg.Tenant.Prefix is not set it won't conflict with it. If it's set then they can co-exist I guess.
There was a problem hiding this comment.
Hm, yeah I hadn't considered that. I could just override p.cfg.Tenant.Prefix when PrefixPreferSource is set. That'd save a lot of these changes.
I'll make those changes and will validate in my setup here.
There was a problem hiding this comment.
I've made these changes, and tested them in my setup and all looks good. And yeah, this dramatically simplifies the change. Thanks for pushing on this one!
There was a problem hiding this comment.
OK, I believe I have addressed your concerns in 2e8c2fa. Please confirm.
| @@ -149,6 +149,12 @@ tenant: | |||
| # https://grafana.com/docs/mimir/latest/configure/about-tenant-ids/ | |||
| # env: CT_TENANT_PREFIX | |||
| prefix: foobar- | |||
|
|
|||
| # If true will use the tenant ID of the inbound request as the prefix of the new tenant id. | |||
There was a problem hiding this comment.
I think we need a bit more detailed description, maybe with a example etc. I didn't get it at first :) Maybe clarify that it takes X-Org-Id header from incoming request, concatenates etc
There was a problem hiding this comment.
I've added an example inline. Do you think that's sufficient? If not I can add a more detailed example further down in the README.
|
I'm not 100% convinced it's the functionality we need, but if that helps somebody and doesn't complicate code much - why not. Conflicts need to be resolved after merging prev PR |
matthewjstanford
force-pushed
the
prefix_prefer_source
branch
from
e1bf9ec
to
ef23a6e
Compare
| @@ -232,20 +232,6 @@ func Test_request_headers(t *testing.T) { | |||
| assert.Equal(t, "my-tenant", string(req.Header.Peek("X-Scope-OrgID"))) | |||
| } | |||
|
|
|||
| func Test_request_headers_with_prefix(t *testing.T) { | |||
There was a problem hiding this comment.
This test is no longer worthwhile, since there is no logic in the fillRequestHeaders() method.
processor.go
Outdated
| res[idx] = r | ||
| }(i, tenant, wrReq) | ||
| }(i, tenantPrefix, tenant, wrReq) |
There was a problem hiding this comment.
maybe just concat tenant+tenantPrefix here and not pass down to goroutine and send() as a parameter? if prefix is empty it won't make a difference
|
I guess it's now good, thanks! |
Adding the ability to use the source requests tenant context as the prefix in the new tenant id.
Resolves #60