Enforce max span attribute size #4335
Conversation
ie-pham
requested review from
joe-elliott,
mdisibio,
mapno,
yvrhdn,
zalegrala,
electron0zero,
stoewer,
javiermolinar and
knylander-grafana
as code owners
ie-pham
changed the title
modules/distributor/distributor.go
Outdated
|
|
||
| func spanContainsAttributeTooLarge(span *v1.Span, maxAttrSize int) bool { | ||
| for _, attr := range span.Attributes { | ||
| if len(attr.Key) > maxAttrSize || attr.Value.Size() > maxAttrSize { |
There was a problem hiding this comment.
Good call, checking the attribute name. I'm wondering if this calculation should be against the total Key.Size() + Value.Size()?
There was a problem hiding this comment.
if key is max/2 and value is max/2, does it still cause a problem?
modules/distributor/distributor.go
Outdated
|
|
||
| for _, b := range batches { | ||
| spansByILS := make(map[uint32]*v1.ScopeSpans) | ||
|
|
||
| for _, ils := range b.ScopeSpans { | ||
| for _, span := range ils.Spans { | ||
| // drop spans with attributes that are too large | ||
| if spanContainsAttributeTooLarge(span, maxSpanAttrSize) { |
There was a problem hiding this comment.
If there is a way we could drop only the attribute that seems better than dropping the span (preserve as much information as possible). It doesn't fit as easily within our current metrics/logs though.
| @@ -229,6 +229,10 @@ distributor: | |||
| # instruct the client how to retry. | |||
| [retry_after_on_resource_exhausted: <duration> | default = '0' ] | |||
|
|
|||
| # Optional | |||
| # Configures the max size a span attribute can be. Any span with at least one attribute over this limit would be discarded with reason "attribute_too_large" | |||
| [max_span_attr_size: <int> | default = '10000'] | |||
There was a problem hiding this comment.
Thinking if this should be max_attr_bytes to follow the pattern of other limits, and should it also apply to resource/event/link attributes?
|
benchmark updated but not looking too good :/ |
ie-pham
changed the title
modules/distributor/distributor.go
Outdated
| if err != nil { | ||
| overrides.RecordDiscardedSpans(spanCount, reasonInternalError, userID) | ||
| logDiscardedResourceSpans(batches, userID, &d.cfg.LogDiscardedSpans, d.logger) | ||
| return nil, err | ||
| } | ||
|
|
||
| if truncatedAttributeCount > 0 { | ||
| level.Warn(d.logger).Log("msg", fmt.Sprintf("truncated %d resource/span attributes when adding spans for tenant %s", truncatedAttributeCount, userID)) |
There was a problem hiding this comment.
error message doesn't tell us anything that the is not already in the metric. remove? if we add some details that make it worth keeping i think we should consider dropping to debug b/c this could get spammy
What this PR does: Add config for
max_span_attr_sizeto enforce the max size a span attribute can be. If a span contains an attribute with either key or value over the limit is sent to Tempo, the attribute key/value will be truncated.Which issue(s) this PR fixes:
Fixes #3985
benchmarks
4 traces, 300 batches and 5000 spans each run
with adding "_truncated"
without adding "_truncated" to truncated attribute values
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]