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

Replace Record limit methods with DroppedAttributes #5190

Merged
merged 18 commits into from
Apr 16, 2024

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Apr 10, 2024

Resolve #5186

Similar to the trace SDK, de-duplicate the attributes before dropping.

Note, this does not include any truncation of string and string slice attributes. That is left for a follow-up PR.

Follow-up Tasks

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:logs Part of OpenTelemetry logs labels Apr 10, 2024
@MrAlias MrAlias force-pushed the record-dropped branch 3 times, most recently from 076a3af to 30c5958 Compare April 10, 2024 18:55
Copy link

codecov bot commented Apr 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.3%. Comparing base (1ff2e71) to head (9c3e01b).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5190   +/-   ##
=====================================
  Coverage   84.3%   84.3%           
=====================================
  Files        258     258           
  Lines      16974   17042   +68     
=====================================
+ Hits       14313   14383   +70     
+ Misses      2362    2360    -2     
  Partials     299     299           
Files Coverage Δ
exporters/stdout/stdoutlog/record.go 100.0% <100.0%> (ø)
sdk/log/record.go 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@MrAlias MrAlias marked this pull request as ready for review April 10, 2024 20:24
sdk/log/record.go Outdated Show resolved Hide resolved
sdk/log/record.go Outdated Show resolved Hide resolved
sdk/log/record.go Outdated Show resolved Hide resolved
sdk/log/record.go Outdated Show resolved Hide resolved
Comment compactAttr and deduplicate.
@pellared
Copy link
Member

I just want to say that I support the API changes (addition of DroppedAttributes and removal of AttributeCountLimit and AttributeValueLengthLimit).

I had no time to review this PR yet.

sdk/log/record.go Outdated Show resolved Hide resolved
Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need benchmarks for AddAttributes and SetAttributes in the following tasks?

@MrAlias
Copy link
Contributor Author

MrAlias commented Apr 16, 2024

Do we need benchmarks for AddAttributes and SetAttributes in the following tasks?

We do. That is left to #5054

@MrAlias MrAlias merged commit dbe27d4 into open-telemetry:main Apr 16, 2024
27 checks passed
@MrAlias MrAlias deleted the record-dropped branch April 16, 2024 18:48
@MrAlias MrAlias added this to the v1.26.0 milestone Apr 19, 2024
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 pkg:SDK Related to an SDK package
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Dropped attribute count is not recorded by log record
4 participants