Make Backend a MemoryBackend of a Backend interface #362
Conversation
sampler/backend.go
Outdated
| CountSignature(signature Signature) | ||
|
|
||
| // GetTotalScore returns the TPS of all traces ingested | ||
| GetTotalScore() float64 |
There was a problem hiding this comment.
Would be awesome if we could rid ourselves of the Get* prefix for these methods. In "Effective Go" you can see in the corresponding section that it is advised to not use this prefix in Go. Unless there is a good reason for it, would be great to remove it.
There was a problem hiding this comment.
Absolutely and that's my plan ; I don't wanted to mix goals so I wanted to keep it for a future PR.
There was a problem hiding this comment.
If you want, I am happy to make these changes for you within this PR.
sampler/backend.go
Outdated
| // GetAllSignatureScores returns the TPS of traces ingested for all signatures | ||
| GetAllSignatureScores() map[Signature]float64 | ||
|
|
||
| // get the number of different signatures seen |
There was a problem hiding this comment.
Please start the documentation name with the name of the method, e.g.:
GetCardinality returns the number of distinct signatures seen.
sampler/backend.go
Outdated
| GetSampledScore() float64 | ||
| // GetUpperSampledScore is similar to GetSampledScore, but with the upper approximation | ||
| GetUpperSampledScore() float64 | ||
| // GetSignatureScore returns the TPS of traces ingested of a given signature |
There was a problem hiding this comment.
Unless the TPS concept is explained elsewhere in another doc, I think it would be great to say "traces per second" instead of TPS here, at least once.
sampler/backend.go
Outdated
|
|
||
| // CountSample counts that 1 trace is going through the sampler | ||
| CountSample() | ||
| // CountSignature counts that 1 signature is going through the sampler |
There was a problem hiding this comment.
In some places throughout the definition of this interface you've left a blank line between the method and the documentation of the next method. Please try and keep consistency. As a personal preference I prefer to have the blank line, but you can chose whichever version you like as long as it's consistent (ideally throughout the package but at the minimum on the interface level).
There was a problem hiding this comment.
Right, they were grouped by "nature", but that's not explicit and a bit arbitrary.
sampler/backend.go
Outdated
| b.totalScore /= b.decayFactor | ||
| b.sampledScore /= b.decayFactor | ||
| b.mu.Unlock() | ||
| // Backend stores and count traces and signatures ingested by a sampler |
There was a problem hiding this comment.
s/count/counts
Would be awesome if we could end each sentence with a period.
sampler/memory_backend.go
Outdated
| func (b *MemoryBackend) CountSignature(signature Signature) { | ||
| b.mu.Lock() | ||
| b.scores[signature]++ | ||
| b.totalScore++ |
There was a problem hiding this comment.
You could use sync/atomic both here and in the method below.
There was a problem hiding this comment.
Right, the use of atomic could be considered for most of the unique counters of the MemoryBackend. I don't want to mix purposes in this PR but that's certainly something which could be addressed (with benchmarks?) in future work.
|
|
||
| // GetSignatureScore returns the score of a signature. | ||
| // It is normalized to represent a number of signatures per second. | ||
| func (b *MemoryBackend) GetSignatureScore(signature Signature) float64 { |
There was a problem hiding this comment.
This can be simplified without the local variable, but I don't feel strongly about it:
b.mu.RLock()
defer b.mu.RUnlock()
return b.scores[signature] / b.countScaleFactorThere was a problem hiding this comment.
You may do the same in all the below methods to get rid of the extra local variable.
There was a problem hiding this comment.
I have no strong opinion either.
At some places we aren't using defer, and from what I recall it isn't to micro-optimize but mostly because they are trivial situations that don't require it.
I'll be fine if we change it in a way or another in the future.
sampler/backend.go
Outdated
| // GetSignatureScore returns the TPS of traces ingested of a given signature | ||
| GetSignatureScore(signature Signature) float64 | ||
| // GetAllSignatureScores returns the TPS of traces ingested for all signatures | ||
| GetAllSignatureScores() map[Signature]float64 |
There was a problem hiding this comment.
It think it would be no problem nor would it make it more confusing to remove "All" from this name.
sampler/memory_backend.go
Outdated
|
|
||
| // GetCardinality returns the number of different signatures seen recently. | ||
| func (b *MemoryBackend) GetCardinality() int64 { | ||
| b.mu.Lock() |
There was a problem hiding this comment.
I think this was meant to be a "read lock".
sampler/memory_backend.go
Outdated
| b.mu.Lock() | ||
| for sig := range b.scores { | ||
| score := b.scores[sig] | ||
| if score > b.decayFactor*minSignatureScoreOffset { |
There was a problem hiding this comment.
Since you only use score here, on this condition, it might be easier as:
if score := b.scores[sig]; score > b.decayFactor*minSignatureScoreOffset {...or even not having it at all
LotharSee
force-pushed
the
benjamin/sampler-backend-interface
branch
from
6d2757a to
e94ef0c
Compare
Make
Backendan interface.It should make the code more explicit about what it exposes, and will allow a better reusability/flexibility.