Flow tenant through processors as a string #3661
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3661 +/- ##
==========================================
- Coverage 96.53% 96.48% -0.05%
==========================================
Files 267 268 +1
Lines 15653 15668 +15
==========================================
+ Hits 15111 15118 +7
- Misses 453 459 +6
- Partials 89 91 +2
Continue to review full report at Codecov.
|
|
@esnible is the intention here to make the writers tenant aware or have a writer per tenant and route to the appropriate writer in the span processor? |
|
@pavolloffay The intention is to have the writers be tenant-aware. @yurishkuro said "I see benefits to having per-tenant queues for better isolation" but later said "My suggestion would be to start with the simplest form: everything identical and shared, but tenancy exists at the data level." This is an attempt to get the tenant down to the SpanWriter with as little churn as possible as a first PR. |
storage/spanstore/interface.go
Outdated
| @@ -86,3 +86,11 @@ type Operation struct { | |||
| Name string | |||
| SpanKind string | |||
| } | |||
|
|
|||
| // tenantKeyType is a custom type for the key "tenant", following context.Context convention | |||
There was a problem hiding this comment.
I would prefer not to expose the constant, but to expose WithTenant(ctx, tenant) and GetTenant(ctx) functions, and make the constant private.
There was a problem hiding this comment.
also, these should probably be defined at storage/ level, not storage/spanstore
cmd/collector/app/span_processor.go
Outdated
| @@ -145,7 +146,8 @@ func (sp *spanProcessor) saveSpan(span *model.Span) { | |||
|
|
|||
| startTime := time.Now() | |||
| // TODO context should be propagated from upstream components | |||
There was a problem hiding this comment.
| // TODO context should be propagated from upstream components | |
| // Since we save spans asynchronously from receiving them, we cannot reuse | |
| // the inbound Context, as it may be cancelled by the time we reach this point, | |
| // so we need to start a new Context. |
storage/tenant.go
Outdated
| return "" | ||
| } | ||
|
|
||
| return tenant.(string) |
There was a problem hiding this comment.
perhaps we could make it safer
if s, ok := tenant.(string) {
return s
}
return ""
esnible
force-pushed
the
flow-tenant-as-string
branch
from
8f00dc7
to
6b39ca4
Compare
| // Verify that the dummy tenant from SpansOptions context made it to writer | ||
| assert.Equal(t, true, w.tenants[dummyTenant]) | ||
| // Verify no other tenantKey context values made it to writer | ||
| assert.True(t, reflect.DeepEqual(w.tenants, map[string]bool{dummyTenant: true})) |
There was a problem hiding this comment.
I think you can simply use assert.Equal(), it should compare the maps correctly iirc.
Signed-off-by: Ed Snible <snible@us.ibm.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
esnible
force-pushed
the
flow-tenant-as-string
branch
from
6b39ca4
to
b01e658
Compare
* Flow tenant through processors as a string Signed-off-by: Ed Snible <snible@us.ibm.com> * GetTenant()/WithTenant() functions Signed-off-by: Ed Snible <snible@us.ibm.com> * Tests for WithTenant()/GetTenant() Signed-off-by: Ed Snible <snible@us.ibm.com> Signed-off-by: Albert Teoh <see.kwang.teoh@gmail.com>
This PR replaces #3659 which passed the tenant ID string as part of a context. It is intended as the first part of a true replacement for the proof-of-concept #3605
cc @pavolloffay