Conversation
yuchen-db
force-pushed
the
yuchen-db/legacy-tenant-attribution
branch
from
e1c1011 to
81abd84
Compare
jnyi
left a comment
jnyi
left a comment
There was a problem hiding this comment.
we might want to do an integration test in a dev region
| type tenantRequestStats map[string]requestStats | ||
|
|
||
| func (h *Handler) handleRequest(ctx context.Context, rep uint64, tenantHTTP string, wreq *prompb.WriteRequest) (tenantRequestStats, error) { | ||
| func (h *Handler) handleRequest(ctx context.Context, rep uint64, tenantHTTP string, httpHeaderPresent bool, wreq *prompb.WriteRequest) (tenantRequestStats, error) { |
There was a problem hiding this comment.
why introduce new variable, can tenantHTTP string tell if a header is present? I think if it isn't, it will be empty?
There was a problem hiding this comment.
this is obtained in tenantHTTP, err := tenancy.GetTenantFromHTTP(r, h.options.TenantHeader, h.options.DefaultTenantID, h.options.TenantField)
a few places might be broken for example,
rate limit logic: if !requestLimiter.AllowSizeBytes(tenantHTTP, int64(len(reqBuf))) {
e2e latency calculation: h.writeE2eLatency.WithLabelValues(strconv.Itoa(responseStatusCode), tenantHTTP, strconv.FormatBool(isPreAgged)).Observe(lat)
with new tenant attribution, we might need to fix how those work too, (especially for rate limit which is request scoped, it might be not possible to reject 1 remote write directly anymore)
| } | ||
|
|
||
| // Tenant attribution logic | ||
| if h.options.TenantAttributor != nil { |
There was a problem hiding this comment.
also note this is exclusive to h.splitTenantLabelName != ""
| @@ -914,6 +920,7 @@ func (h *Handler) fanoutForward(ctx context.Context, params remoteWriteParams) ( | |||
| // series that should be written to remote nodes. | |||
| func (h *Handler) distributeTimeseriesToReplicas( | |||
| tenantHTTP string, | |||
There was a problem hiding this comment.
wonder if we can use tenantHTTP to tell the diff, we should set it to empty string if HTTP headers didn't set it and set to default TenantId within this function
jnyi
left a comment
jnyi
left a comment
There was a problem hiding this comment.
would suggest to update GetTenantFromHTTPfunction, default would be unset if we enable these flags:
--receive.split-tenant-label-name
or
--receive.tenant-rules + --receive.no-verify-tenant-attribution
2 participants
(generated by Claude)
New Files Created
- Ported M3's filter package (glob-style pattern matching)
- TenantAttributor struct with config loading and tenant matching
- Supports patterns: , prefix, *suffix, contains, ?, [abc], [a-z], {a,b,c}, !negation
- Verification mode metrics (attributionMatches, attributionMismatches)
- Unit tests for filter parsing, pattern matching, TenantAttributor, verification mode
Modified Files
- Added --receive.tenant-rules flag (path to YAML config)
- Added --receive.verify-tenant-attribution flag (verification mode)
- TenantAttributor initialization before handler creation
- Added TenantAttributor to Options struct
- Integrated tenant attribution in distributeTimeseriesToReplicas
- In verification mode: computes attribution for every series, records match/mismatch metrics, but keeps using HTTP tenant for routing
- In normal mode: only attributes when no HTTP header is present
Behavior Matrix
Changes
Verification