[exporter/datadog]: add max tag length #3185
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3185 +/- ##
==========================================
+ Coverage 91.91% 91.92% +0.01%
==========================================
Files 494 494
Lines 23939 23956 +17
==========================================
+ Hits 22003 22022 +19
+ Misses 1429 1428 -1
+ Partials 507 506 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| // TruncateUTF8 truncates the given string to make sure it uses less than limit bytes. | ||
| // If the last character is an utf8 character that would be splitten, it removes it | ||
| // entirely to make sure the resulting string is not broken. | ||
| func TruncateUTF8(s string, limit int) string { |
There was a problem hiding this comment.
Maybe we should add some tests for this function (just so that Collector maintainers have an accurate coverage measure)
| if len(v) > MaxMetaValLen { | ||
| v = utils.TruncateUTF8(v, MaxMetaValLen) + "..." | ||
| } | ||
|
|
There was a problem hiding this comment.
Why do we want to do this only when it's not a service name/span type/analytics event? Some of those can have arbitrary values, right? Why not do this every time?
There was a problem hiding this comment.
yea, idk, i rushed things. should be for everything, i'll update
| // ensure that truncation helperr function truncates strings as expected | ||
| // and accounts for the limit and multi byte ending characters | ||
| // from https://github.com/DataDog/datadog-agent/blob/140a4ee164261ef2245340c50371ba989fbeb038/pkg/trace/traceutil/truncate_test.go#L15 | ||
| func TestTruncateUTF8Strings(t *testing.T) { |
There was a problem hiding this comment.
I think codecov doesn't see this test because it's not on the utils package (where it arguably should be). Can you move it there?
There was a problem hiding this comment.
updated (along with other pr), I think codecov found it?
|
@bogdandrutu Could i get a quick review here when u have a moment? 🙇 |
|
Please resolve the conflicts. |
|
@tigrannajaryan resolved! |
This PR is part of work to address Payload size errors reported here: open-telemetry#2676 Combined with batching of spans/traces, spans with very large attribute values such as stacktraces can cause payload sizes to become excessive. This PR adds a max tag (a datadog span attribute, basically), value helper function, and truncates tag values when they're too large, which brings this in line with Datadog-Agent limits: https://github.com/DataDog/datadog-agent/blob/140a4ee164261ef2245340c50371ba989fbeb038/pkg/trace/traceutil/truncate.go#L23 Additionally, worth noting that work being done to directly import and re-use portions of the datadog-agent codebase will make this sort of thing much easier, and work being done within datadog to permit shorter batching times should also help resolve the Payload size errors being seen. However for now this PR can help. **Link to tracking Issue:** <Issue number if applicable> open-telemetry#2676
Description:
This PR is part of work to address Payload size errors reported here: #2676
Combined with batching of spans/traces, spans with very large attribute values such as stacktraces can cause payload sizes to become excessive. This PR adds a max tag (a datadog span attribute, basically), value helper function, and truncates tag values when they're too large, which brings this in line with Datadog-Agent limits: https://github.com/DataDog/datadog-agent/blob/140a4ee164261ef2245340c50371ba989fbeb038/pkg/trace/traceutil/truncate.go#L23
Additionally, worth noting that work being done to directly import and re-use portions of the datadog-agent codebase will make this sort of thing much easier, and work being done within datadog to permit shorter batching times should also help resolve the Payload size errors being seen. However for now this PR can help.
Link to tracking Issue:
#2676
Testing:
Add unit tests
Documentation:
n/a