[v2][WIP] Span hash sanitizer and enhance span hash adjuster #6499
Conversation
Signed-off-by: chahatsagarmain <[email protected]>
| hashStr := hex.EncodeToString(buf.Bytes()) | ||
| span.Tags = append(span.Tags, model.KeyValue{ | ||
| Key: "span.hash", | ||
| VType: model.ValueType_STRING, |
There was a problem hiding this comment.
hard to evaluate this PR without seeing how it will be used in the storage backends. For example, is it really a string we want, or a number?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6499 +/- ##
==========================================
- Coverage 96.27% 96.17% -0.10%
==========================================
Files 372 373 +1
Lines 21282 21332 +50
==========================================
+ Hits 20490 20517 +27
- Misses 605 622 +17
- Partials 187 193 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: chahatsagarmain <[email protected]>
model/span.go
Outdated
| func (s *Span) HashHashTag() bool { | ||
| if _, ok := KeyValues(s.Tags).FindByKey(SpanHashKey); ok { | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
| func (s *Span) HashHashTag() bool { | |
| if _, ok := KeyValues(s.Tags).FindByKey(SpanHashKey); ok { | |
| return true | |
| } | |
| return false | |
| } |
redundant. Someone can always call _, ok := GetHashTag
| return span | ||
| } | ||
|
|
||
| func uint64ToInt64Bits(value uint64) int64 { |
There was a problem hiding this comment.
duplication. I would move this to Span, e.g. span.SetHashTag()
Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
| spanHash, _ := model.HashCode(span) | ||
| spanHash, found := span.GetHashTag() | ||
| if !found { | ||
| tempSpam := *span |
There was a problem hiding this comment.
why do we need to make a copy?
There was a problem hiding this comment.
I wanted a review on this , calling SetHashTag sets the span.hash for the span tag and unit tests fail because of unexpected value .
There was a problem hiding this comment.
yes, I would expect the test to fail if we add a new tag, depending on how the test are implemented. We need to work around that.
model/span.go
Outdated
| if tag, ok := KeyValues(s.Tags).FindByKey(SpanHashKey); ok { | ||
| return tag.GetVInt64(), true | ||
| } | ||
| return -1, false |
There was a problem hiding this comment.
| return -1, false | |
| return 0, false |
I don't thin -1 conveys any more meaning than 0, so I would rather return default value, this would match behavior of a get from standard map[string]int64
model/span.go
Outdated
| return -1, err | ||
| } | ||
| spanHash := uint64ToInt64Bits(hCode) | ||
| s.Tags = append(s.Tags, KeyValue{ |
There was a problem hiding this comment.
we have helper methods for creating typed instances of KeyValue, please use that instead of explicit creation.
| @@ -16,6 +16,7 @@ type SanitizeSpan func(span *model.Span) *model.Span | |||
| func NewStandardSanitizers() []SanitizeSpan { | |||
| return []SanitizeSpan{ | |||
| NewEmptyServiceNameSanitizer(), | |||
| NewHashingSanitizer(), | |||
There was a problem hiding this comment.
note: we also have v2 sanitizers, need to add this logic that as well.
./cmd/jaeger/internal/sanitizer
Signed-off-by: chahatsagarmain <[email protected]>
| @@ -27,6 +27,7 @@ const ( | |||
| BinaryType = ValueType_BINARY | |||
|
|
|||
| SpanKindKey = "span.kind" | |||
| SpanHashKey = "span.hash" | |||
There was a problem hiding this comment.
let's move it into a separate group and use value @jaeger@hash - see ##6522
| span.CopyTo(dedupedSpans.AppendEmpty()) | ||
| continue | ||
|
|
||
| var spanHash int64 |
There was a problem hiding this comment.
this should be a helper function (or two) in jptrace package, similar to how you added them to model/
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test