Tenancy for memory storage #3827
Conversation
Signed-off-by: Ed Snible <[email protected]>
Signed-off-by: Ed Snible <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #3827 +/- ##
==========================================
- Coverage 97.58% 97.57% -0.01%
==========================================
Files 291 291
Lines 16913 16938 +25
==========================================
+ Hits 16504 16527 +23
- Misses 323 325 +2
Partials 86 86
Continue to review full report at Codecov.
|
Signed-off-by: Ed Snible <[email protected]>
plugin/storage/memory/memory.go
Outdated
| "github.com/jaegertracing/jaeger/storage/spanstore" | ||
| ) | ||
|
|
||
| // Store is an in-memory store of traces | ||
| type Store struct { | ||
| sync.RWMutex | ||
| config config.Configuration |
There was a problem hiding this comment.
I would recommend keeping config at the Tenant struct level (copy the same one for now), because it seems very reasonable to want per-tenant quota settings rather than one global setting. Or maybe not?
There was a problem hiding this comment.
Sorry, what I meant was this:
type Store struct {
// Each tenant gets a copy of default config.
// In the future this can be extended to contain per-tenant configuration.
defaultConfig config.Config
}
type Tenant struct {
config config.Config
}
func (s *Store) newTenant() *Tenant {
return &Tenant{
config: s.defaultConfig
}
}
plugin/storage/memory/memory.go
Outdated
| } | ||
| } | ||
|
|
||
| // GetTenant returns the per-tenant storage. Note that tenantID has already been checked for by the collector or query | ||
| func (st *Store) GetTenant(tenantID string) *Tenant { |
There was a problem hiding this comment.
does this need to be public?
Signed-off-by: Ed Snible <[email protected]>
plugin/storage/memory/memory.go
Outdated
| "github.com/jaegertracing/jaeger/storage/spanstore" | ||
| ) | ||
|
|
||
| // Store is an in-memory store of traces | ||
| type Store struct { | ||
| sync.RWMutex | ||
| config config.Configuration |
There was a problem hiding this comment.
Sorry, what I meant was this:
type Store struct {
// Each tenant gets a copy of default config.
// In the future this can be extended to contain per-tenant configuration.
defaultConfig config.Config
}
type Tenant struct {
config config.Config
}
func (s *Store) newTenant() *Tenant {
return &Tenant{
config: s.defaultConfig
}
}
plugin/storage/memory/memory.go
Outdated
| func (st *Store) getTenant(tenantID string) *Tenant { | ||
| tenant, ok := st.perTenant[tenantID] | ||
| if !ok { | ||
| // We do the lookup twice to skip locking on retrieval of existing tenant |
There was a problem hiding this comment.
Can't really do this because you're doing unprotected read above that could cause a concurrency issue. Test with -race should catch this. If you're worried about locking, you can either use RWLock or use a sync.Map which is specifically designed for frequent reads / rare writes.
Signed-off-by: Ed Snible <[email protected]>
plugin/storage/memory/memory.go
Outdated
|
|
||
| func (st *Store) newTenant() *Tenant { | ||
| return &Tenant{ | ||
| ids: make([]*model.TraceID, st.defaultConfig.MaxTraces), |
There was a problem hiding this comment.
nit: can decouple this func from the knowledge of defaultConfig:
// caller
newTenant(st.defaultConfig)
func (st *Store) newTenant(cfg config.Config) *Tenant {
// ...
}
| @@ -290,7 +330,7 @@ func (m *Store) validSpan(span *model.Span, query *spanstore.TraceQueryParameter | |||
| return true | |||
| } | |||
|
|
|||
| func (m *Store) flattenTags(span *model.Span) model.KeyValues { | |||
| func flattenTags(span *model.Span) model.KeyValues { | |||
| retMe := span.Tags | |||
| retMe = append(retMe, span.Process.Tags...) | |||
There was a problem hiding this comment.
this looks like existing bug - it could mutate span.Tags
plugin/storage/memory/memory_test.go
Outdated
| @@ -53,6 +54,32 @@ var testingSpan = &model.Span{ | |||
| StartTime: time.Unix(300, 0).UTC(), | |||
| } | |||
|
|
|||
| var traceID2 = model.NewTraceID(2, 3) | |||
|
|
|||
| var testingSpan2 = &model.Span{ | |||
There was a problem hiding this comment.
nit: turn testingSpan into a function that takes a few args distinguishing the traces (like trace id)
Signed-off-by: Ed Snible <[email protected]>
Signed-off-by: Ed Snible [email protected]
This PR adds tenancy support to the memory storage.