Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sdk/log record attr value limit #6032

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Dec 11, 2024

Fix #6004

Copy of #5997

Correctness

From the OTel specification:

  • set an attribute value length limit such that for each attribute value:
    • if it is a string, if it exceeds that limit (counting any character in it as 1), SDKs MUST truncate that value, so that its length is at most equal to the limit...

Our current implementation truncates on number of bytes not characters.

Unit tests are added/updated to validate this fix and prevent regressions.

Performance

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/log
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
                        │ commit-e9c7aac2(old).txt │       commit-878043b9(new).txt       │
                        │          sec/op          │    sec/op     vs base                │
Truncate/Unlimited-8                 0.8323n ±  3%   0.7367n ± 3%  -11.49% (p=0.000 n=10)
Truncate/Zero-8                       1.923n ± 32%    1.359n ± 2%  -29.34% (p=0.000 n=10)
Truncate/Short-8                    14.6050n ±  4%   0.8785n ± 1%  -93.98% (p=0.000 n=10)
Truncate/ASCII-8                      8.205n ±  2%    3.601n ± 7%  -56.12% (p=0.000 n=10)
Truncate/ValidUTF-8-8                11.335n ±  1%    7.206n ± 1%  -36.43% (p=0.000 n=10)
Truncate/InvalidUTF-8-8               58.26n ±  1%    36.61n ± 1%  -37.17% (p=0.000 n=10)
Truncate/MixedUTF-8-8                 81.16n ±  1%    52.30n ± 1%  -35.56% (p=0.000 n=10)
geomean                               10.04n          4.601n       -54.16%

                        │ commit-e9c7aac2(old).txt │      commit-878043b9(new).txt       │
                        │           B/op           │    B/op     vs base                 │
Truncate/Unlimited-8                  0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Truncate/Zero-8                       0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Truncate/Short-8                      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Truncate/ASCII-8                      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Truncate/ValidUTF-8-8                 0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Truncate/InvalidUTF-8-8               16.00 ± 0%     16.00 ± 0%       ~ (p=1.000 n=10) ¹
Truncate/MixedUTF-8-8                 32.00 ± 0%     32.00 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                          ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                        │ commit-e9c7aac2(old).txt │      commit-878043b9(new).txt       │
                        │        allocs/op         │ allocs/op   vs base                 │
Truncate/Unlimited-8                  0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Truncate/Zero-8                       0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Truncate/Short-8                      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Truncate/ASCII-8                      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Truncate/ValidUTF-8-8                 0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Truncate/InvalidUTF-8-8               1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
Truncate/MixedUTF-8-8                 1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                          ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

@MrAlias MrAlias added this to the v1.33.0 milestone Dec 11, 2024
@MrAlias MrAlias force-pushed the log-attr-val-truncate branch from 3c1df34 to 9fd0b09 Compare December 11, 2024 18:45
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.2%. Comparing base (58fdf2a) to head (878043b).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6032   +/-   ##
=====================================
  Coverage   82.2%   82.2%           
=====================================
  Files        273     273           
  Lines      23647   23674   +27     
=====================================
+ Hits       19449   19476   +27     
  Misses      3851    3851           
  Partials     347     347           

see 1 file with indirect coverage changes

@MrAlias MrAlias force-pushed the log-attr-val-truncate branch from 9fd0b09 to 878043b Compare December 11, 2024 19:17
@MrAlias MrAlias added bug Something isn't working pkg:SDK Related to an SDK package area:logs Part of OpenTelemetry logs labels Dec 11, 2024
@MrAlias MrAlias marked this pull request as ready for review December 11, 2024 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logs Part of OpenTelemetry logs bug Something isn't working pkg:SDK Related to an SDK package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log attributes are truncated by number of bytes not number of characters
3 participants