Multitenant pipeline design#3389
Conversation
github-actions
Bot
added
documentation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3389 +/- ##
==========================================
- Coverage 86.23% 86.22% -0.01%
==========================================
Files 739 739
Lines 292355 292355
==========================================
- Hits 252116 252095 -21
- Misses 39715 39736 +21
Partials 524 524
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds new documentation describing a proposed multitenant “tenant descriptor” model for request context propagation and conditional per-tenant limiter configuration, along with an accompanying flow diagram.
Changes:
- Introduces an
agent-multitenancy-design.mddesign document detailing tenant descriptor resolution, context propagation, and limiter condition evaluation. - Adds an SVG diagram illustrating request/descriptor/context flow and how downstream limiters reuse precomputed lookup keys.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| rust/otap-dataflow/docs/agent-multitenancy-design.md | New design doc describing tenant descriptor modeling, context propagation, and limiter configuration patterns. |
| rust/otap-dataflow/docs/agent-multitenancy-diagram.svg | New diagram visualizing the request-to-context flow and O(1) limiter probing approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| stored. | ||
|
|
||
| Tenant descriptors are multi-dimensional to enable tenant assignment | ||
| in logically independent ways with user control ove rcardinality. For |
| - Reject the configuration: callers asserts single-reource context or else | ||
| invalid configuration. |
| the context: whichever component consumes the context becomes | ||
| responsible releasing it. The detailed Rust ownership mechanics are an | ||
| out of scope here (see | ||
| [otel-arrow#3316](https://github.com/open-telemetry/otel-arrow/pull/3316), | ||
| however the main requirement is that owners are responsible for | ||
| releasing resource reservations and sometimes the reservation is |
| resource holds do not automatically cross shared boundaries (i.e., | ||
| they are `!Send`). Sub-components within the engine (e.g., channels) | ||
| assume responsibility for resources, and special nodes (e.g., topic | ||
| exporter and receiver) will require an explicit mechanisms to transfer |
|
See github.com//issues/3375 |
| stored. | ||
|
|
||
| Tenant descriptors are multi-dimensional to enable tenant assignment | ||
| in logically independent ways with user control ove rcardinality. For |
There was a problem hiding this comment.
| in logically independent ways with user control ove rcardinality. For | |
| in logically independent ways with user control over cardinality. For |
| These actions do not apply in contexts where multiple resources are | ||
| present. Under this proposal, the options for handling multi-resource | ||
| requests with these resource descriptor actions are: | ||
|
|
||
| - Nack the request. | ||
| - Reject the configuration: callers asserts single-reource context or else | ||
| invalid configuration. | ||
| - Do not resolve the action, the descriptor will be unresolved; receivers | ||
| and limiters that require this descriptor will reject, otherwise these | ||
| requests will not match conditions and take the default limit. |
There was a problem hiding this comment.
I think we could use the concept of partitioning described here, if it were placed upstream of the thing that applies these actions, to mitigate the issue of mixed resource batches.
This issue #3273 describes the ability to partition based on the result of some OPL expression. In this case the expression could simply be something like resource.attributes["service.name"].
Note: I imagine the partition-by-expression implementation will need to partition on multiple expressions so we can use the output for multiple actions (this is not called out in #3273, but seems like the right thing to do).
| # first-match wins | ||
| routes: | ||
| # customer_id=bigfish | ||
| - entries: | ||
| - key: customer_id | ||
| value: bigfish | ||
| output: bigfish_pipeline | ||
| # for premium-tier customers | ||
| - entries: | ||
| - key: tier | ||
| value: premium | ||
| output: premium_pipeline | ||
| # for any customer | ||
| - entries: | ||
| - key: customer_id | ||
| output: shared_pipeline | ||
| default_output: fallback |
There was a problem hiding this comment.
Although #3310 only calls out headers, I really intended this to be catchall issue where we added the ability to access / manipulate batch metadata in OPL, and I believe this tenant description is something that could be included. If we did that, I imagine that this kind of tenant based routing could also be expressed in OPL and done through transform processor
signals |
if (tenant_descriptor["customer_id"] == "bigfish" ) {
route to "bigfish_pipeline"
} |
if (tenant_descriptor["tier"] == "premium" ) {
route to "premium_pipeline"
} |
if (tenant_descriptor["customer_id"] != null) {
route_to "shared_pipeline"
} |
route_to "fallback"
albertlockett
left a comment
albertlockett
left a comment
There was a problem hiding this comment.
Really cool @jmacd !
Like we touched on in SIG on Tuesday, I think there's some overlap between what this PR is trying to acheive and some work we have planned in the query-engine/OPL.
Specifcally, I think this capability to partition by an OPL expression (which could be extracted to a consumable function for easy invocation outside the full query-engine context), could be used to split batches with metadata that could be used to drive some tenant description actions:
#3273
I also think it'd be great to add the capability to work with these tenant descriptors into OPL/query-engine (and batch metadata generally), which I'm tracking in:
#3310
| value: production | ||
| ``` | ||
|
|
||
| These actions do not apply in contexts where multiple resources are |
There was a problem hiding this comment.
I may be missing something, but this feels like a core gap. Header-derived descriptors classify the whole request/batch as one tenant, while resource_key is single-resource only. For mixed-tenant batches, could we define the expected behavior here? The options below seem to sit at different levels (config rejection, runtime Nack, or unresolved/default), so it would help to specify which applies when.
| a queue. The semaphore acts as a gate, limiting the amount of weight | ||
| admitted into a logical-admitted state. | ||
|
|
||
| The main resource limiter interface **acquire** returns a reservation |
There was a problem hiding this comment.
I think this needs a clearer rule. This section says a resource hold can live in Context and follow the request, but later says holds are !Send. If the request then crosses a topic or moves to another core, that seems hard to make safe. Can we define whether holds stay out of Context, or get released and re-acquired at those boundaries?
|
|
||
| ### Rate limiters | ||
|
|
||
| The rate limiter interface **limit** returns a reservation object. If |
There was a problem hiding this comment.
This seems inconsistent with the earlier rate-limit definition. If rate tokens are consumed immediately and not returned, then an earlier grant cannot really be “cancelled” when a later limiter fails. I think this needs a clear reserve/commit/cancel rule for correctness, not just as an efficiency follow-up. For example, all limiters could first reserve, then commit only after every limiter grants.
| and semaphore limiters support thread-local use only, so they are | ||
| defined with pipeline-scope. The dataflow engine extension mechanism | ||
| supports requesting shared extensions at wider scopes generally, so | ||
| limiter scope can be thread-local (pipeline), CPU-local (pipeline |
There was a problem hiding this comment.
This scope model (pipeline / group / engine) seems to overlap with the existing resources policy framework in config/src/policy.rs, while the current memory_limiter is restricted to top-level policy only. Can we clarify whether these new per-scope limiters are meant to extend that framework or replace it?
| to avoid leaking tenant details outside their scope. The dataflow | ||
| engine is responsible for enforcing this discipline automatically. | ||
|
|
||
| Pipeline operators are advised not to use unauthenticated request |
There was a problem hiding this comment.
Can we make the trust model a bit more explicit here? If descriptors can come from headers, peer address, or resource attributes, it would help to define which sources are trusted and how they relate to authentication.
| Each of these uses the following dimensions: | ||
|
|
||
| - Condition name | ||
| - Key:values in condition entries of matching bucket |
There was a problem hiding this comment.
Can we clarify the intended metric-label policy for tenant values? Customer IDs, workspace IDs, and subscription IDs may be sensitive or high-cardinality even when bucket count is bounded, so it would help to say whether these values are emitted, hashed, or disabled by default.
|
Thanks for putting this together @jmacd. I like the direction and the model is helpful. I left a few questions where I’m trying to understand the intended integration points better. Once those are clear, I think the design will be easier for implementers to follow consistently. |
| stored. | ||
|
|
||
| Tenant descriptors are multi-dimensional to enable tenant assignment | ||
| in logically independent ways with user control ove rcardinality. For |
There was a problem hiding this comment.
| in logically independent ways with user control ove rcardinality. For | |
| in logically independent ways with user control over cardinality. For |
| Tenant descriptors originate in a series of actions, configured at the | ||
| top level, group-level, and pipeline-level of the dataflow engine | ||
| configuration, since these definitions are shared across pipeline | ||
| groups. Descriptor values refer to one or more tenant associations, |
There was a problem hiding this comment.
I don't understand why we say that "these definitions are shared across pipeline groups" when, at the beginning of the sentence, we say that a tenant descriptor can be defined at the pipeline level. In that case, one would expect the descriptor not to be shared at the group level.
| lookup key for evaluating limiter conditions. Tenant descriptor values | ||
| must be erased when they go out of scope to avoid leaking sensitive | ||
| data across unintended boundaries. |
There was a problem hiding this comment.
I like this concept of boundaries.
lquerel
left a comment
lquerel
left a comment
There was a problem hiding this comment.
Thanks, Josh, for this PR. For now, I'm only doing a partial review, as explained in my last comment. I'll continue tomorrow.
| - `remote_address`, `masked_remote_address`: origin network address, optionally CIDR-masked. | ||
| - `generic_key`: a static, hard-coded key-value. | ||
| - `request_header`: copy a request header value into a descriptor key. | ||
| - `request_header_match`: descriptor key value must match a condition. |
There was a problem hiding this comment.
| - `request_header_match`: descriptor key value must match a condition. | |
| - `request_header_match`: request header value must match a condition. |
Not 100% sure but that seems more coherent for me.
|
|
||
|  | ||
|
|
||
| ### Tenant descriptor actions |
There was a problem hiding this comment.
The term "action" seems very generic here. Why not use the term "extractor" instead, or "definition" that better characterizes what is being done?
Alternatives: "Tenant description extractor", "Tenant descriptor definition", "Tenant descriptor spec", ...
| the keys: | ||
|
|
||
| ```yaml | ||
| actions: |
There was a problem hiding this comment.
Not a big fan of the term actions
| - `request_header`: copy a request header value into a descriptor key. | ||
| - `request_header_match`: descriptor key value must match a condition. |
There was a problem hiding this comment.
In other parts of the project, we use the term "transport header" instead of "request header", which seems more appropriate to me.
| Multiple tenant descriptors per request are supported, enabling | ||
| multiple independent limiters with lower dimensions. For example, a | ||
| request may be accompanied by both an end-user tenant identifier and | ||
| an acting-on-behalf-of tenant identifier. These are logically |
There was a problem hiding this comment.
I think the term "acting-on-behalf-of” should be clarified somewhere.
| ```yaml | ||
| token_bucket: | ||
| allow: 100_000_000 # maximum per interval | ||
| burst: 20_000_000 # maximum individual weight | ||
| interval: 60s # interval duration | ||
| waiters: 100 # max number waiting | ||
| mode: fifo # prioritize fairness | ||
| ``` |
There was a problem hiding this comment.
If I want to apply rate limiters and/or resource limiters per tenant, how should I proceed?
| 1. Use a group- or engine-shared limiter instance. | ||
| 2. Route the data by tenant descriptor to a single pipeline, then use a thread-local limiter instance. | ||
|
|
||
| Both are reasonable options. |
There was a problem hiding this comment.
Is option 1 really reasonable?
I think option 2 is more appropriate for our engine and our constraints. We could imagine a concept of a tenant cell combined with the concept of topics to implement efficient multi-tenancy in our engine. I'll try to define this concept of a tenant cell more clearly later on.
| type: extension:token_bucket_limiter | ||
| config: | ||
| unit: network_bytes/second | ||
| optional_descriptors: [enduser_tenant_modern, enduser_tenant_legacy] | ||
| conditions: | ||
| # This gives each workspace with customer_id=bigfish more allowance. | ||
| - name: bigfish | ||
| entries: | ||
| - key: workspace_id | ||
| - key: customer_id | ||
| value: bigfish | ||
| token_bucket: | ||
| allow: 50_000 # 50KB/s allowance per pipeline PER CORE | ||
| burst: 100_000 # 100KB maximum size PER CORE | ||
| cardinality: | ||
| max_count: 1000 # Up to 1000 workspaces (single customer) |
| capabilities: | ||
| # Multiple rate limiters are listed with numeric sorting key "/N". | ||
| rate_limit/0: obo_rate | ||
| rate_limit/1: customer_rate |
There was a problem hiding this comment.
How does that work when the number of tenant is high/dynamic?
| requests will be cancelled. Specific implementation details are | ||
| out-of-scope, see open questions. | ||
|
|
||
| ### Limiter fairness |
There was a problem hiding this comment.
This document/topic is a big piece of work, so I'll stop my review here for now. I'm sure some of my questions/comments will be answered later, but I hope some of them will remain valid even after a full review.
In any case, this document represents a major specification and design effort. I think we should not hesitate to spend time on it to make sure we're aligned on the approach.
Change Summary
Adds the outline of a design for request-level tenant descriptors and a common mechanism
for defining conditional rate and resource limiter configurations by tenant.
What issue does this PR close?
Fixes #470
.chloggen/*.yamlentrychore(indicated in title)