Skip to content

sdk/log: fix "limit reached" logging and Record.DroppedAttributes#7662

Merged
pellared merged 21 commits intoopen-telemetry:mainfrom
mexirica:bad-log-duplicated-key
Jan 13, 2026
Merged

sdk/log: fix "limit reached" logging and Record.DroppedAttributes#7662
pellared merged 21 commits intoopen-telemetry:mainfrom
mexirica:bad-log-duplicated-key

Conversation

@mexirica
Copy link
Copy Markdown
Contributor

@mexirica mexirica commented Dec 3, 2025

Fixes: #6983

This pull request enhances logging and error reporting in the log record attribute handling code, specifically around dropped attributes and duplicate key-value pairs. The main focus is to provide clearer warnings when key-value pairs are dropped due to duplication, and to ensure warnings are only logged when actual drops occur.

Improved logging and error handling for dropped attributes and duplicate keys:

  • Added a new logKeyValuePairDropped warning (using sync.OnceFunc) to log a warning message when key-value pairs are dropped due to key duplication. This ensures that duplicate key drops are clearly reported.
  • Updated methods (addDropped, setDropped) to only log attribute drop warnings when the number of dropped attributes is greater than zero, preventing unnecessary log spam.

Enhancements to attribute deduplication logic:

  • Inserted calls to logKeyValuePairDropped in multiple places within AddAttributes, SetAttributes, and applyValueLimitsAndDedup to log a warning whenever key-value pairs are dropped due to duplication, ensuring better visibility into attribute handling issues.

Code cleanup:

  • Removed redundant calls to addDropped after deduplication, as the logging is now handled directly where drops occur.

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.1%. Comparing base (b992082) to head (f594bc8).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #7662     +/-   ##
=======================================
- Coverage   86.1%   86.1%   -0.1%     
=======================================
  Files        302     302             
  Lines      22042   22046      +4     
=======================================
+ Hits       18990   18991      +1     
- Misses      2672    2674      +2     
- Partials     380     381      +1     
Files with missing lines Coverage Δ
sdk/log/record.go 100.0% <100.0%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mexirica mexirica changed the title [WIP] Fix bad error log Fix bad error log Dec 4, 2025
@mexirica mexirica marked this pull request as ready for review December 4, 2025 18:30
Copy link
Copy Markdown
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

The new tests looks like created with help of GenAI. Can you check it there is no redundancy and try to minimize the amount of new tests? At the same time I do not want to increase the complexity of existing tests (I think it is better to have more tests then fewer that are more complex).

Comment thread sdk/log/record_test.go
Comment thread sdk/log/record.go Outdated
Comment thread sdk/log/record_test.go Outdated
Comment thread sdk/log/record_test.go Outdated
Comment thread sdk/log/record_test.go Outdated
@pellared
Copy link
Copy Markdown
Member

@mexirica , are you able to address the comments?

@mexirica
Copy link
Copy Markdown
Contributor Author

@mexirica , are you able to address the comments?

Hi, sorry for the delay, Will fix they

@mexirica
Copy link
Copy Markdown
Contributor Author

The new tests looks like created with help of GenAI. Can you check it there is no redundancy and try to minimize the amount of new tests? At the same time I do not want to increase the complexity of existing tests (I think it is better to have more tests then fewer that are more complex).

Reduced the number of tests. If that's still not okay, I can clean up more.

@flc1125 flc1125 mentioned this pull request Dec 29, 2025
Comment thread CHANGELOG.md Outdated
Comment thread sdk/log/record.go Outdated
Comment thread sdk/log/record_test.go Outdated
@pellared
Copy link
Copy Markdown
Member

pellared commented Jan 8, 2026

@mexirica, kindly remainder 😉

Also Happy New Year 🎉

@mexirica
Copy link
Copy Markdown
Contributor Author

mexirica commented Jan 8, 2026

@mexirica, kindly remainder 😉

Also Happy New Year 🎉

Happy New Year for you too, excuse me for the delay

Copy link
Copy Markdown
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

@mexirica, last suggestions from my side (I already applied them). I don’t want to gold-plate this any further. Thanks a lot for your work.

@open-telemetry/go-approvers, could you please take a look? I do not think we need a separate PR for each bug.
Fixing one bug was impacting/causing next one. Also reviewing code that contained bugs was very hard to reason about. It would be great if this could land in the upcoming release (hopefully next week).

Comment thread sdk/log/record_test.go Outdated
Comment thread sdk/log/record_test.go Outdated
Comment thread sdk/log/record_test.go
Comment thread sdk/log/record_test.go Outdated
@pellared pellared added this to the v1.40.0 milestone Jan 9, 2026
@pellared pellared changed the title Fix bad error log sdk/log: fix "limit reached" logging and DroppedAttributes Jan 9, 2026
@pellared pellared changed the title sdk/log: fix "limit reached" logging and DroppedAttributes sdk/log: fix "limit reached" logging and Record.DroppedAttributes Jan 9, 2026
@mexirica
Copy link
Copy Markdown
Contributor Author

mexirica commented Jan 9, 2026

@mexirica, last suggestions from my side (I already applied them). I don’t want to gold-plate this any further. Thanks a lot for your work.

@open-telemetry/go-approvers, could you please take a look? I do not think we need a separate PR for each bug. Fixing one bug was impacting/causing next one. Also reviewing code that contained bugs was very hard to reason about. It would be great if this could land in the upcoming release (hopefully next week).

Thank you and sorry for the time i took to resolve the issue.

@pellared
Copy link
Copy Markdown
Member

pellared commented Jan 9, 2026

sorry for the time i took to resolve the issue.

No need to feel sorry. I am really grateful 🏅

@pellared
Copy link
Copy Markdown
Member

pellared commented Jan 9, 2026

Planning to merge on Monday.

@pellared pellared merged commit f98af48 into open-telemetry:main Jan 13, 2026
33 checks passed
@MrAlias MrAlias mentioned this pull request Feb 2, 2026
MrAlias added a commit that referenced this pull request Feb 2, 2026
### Added

- Add `Enabled` method to all synchronous instrument interfaces
(`Float64Counter`, `Float64UpDownCounter`, `Float64Histogram`,
`Float64Gauge`, `Int64Counter`, `Int64UpDownCounter`, `Int64Histogram`,
`Int64Gauge`,) in `go.opentelemetry.io/otel/metric`. This stabilizes the
synchronous instrument enabled feature, allowing users to check if an
instrument will process measurements before performing computationally
expensive operations. (#7763)
- Add `AlwaysRecord` sampler in `go.opentelemetry.io/otel/sdk/trace`.
(#7724)
- Add `go.opentelemetry.io/otel/semconv/v1.39.0` package. The package
contains semantic conventions from the `v1.39.0` version of the
OpenTelemetry Semantic Conventions. See the [migration
documentation](https://github.com/open-telemetry/opentelemetry-go/blob/298cbedf256b7a9ab3c21e41fc5e3e6d6e4e94aa/semconv/v1.39.0/MIGRATION.md)
for information on how to upgrade from
`go.opentelemetry.io/otel/semconv/v1.38.0.` (#7783, #7789)

### Changed

- `Exporter` in `go.opentelemetry.io/otel/exporter/prometheus` ignores
metrics with the scope `go.opentelemetry.io/contrib/bridges/prometheus`.
This prevents scrape failures when the Prometheus exporter is
misconfigured to get data from the Prometheus bridge. (#7688)
- Improve performance of concurrent histogram measurements in
`go.opentelemetry.io/otel/sdk/metric`. (#7474)
- Add experimental observability metrics in
`go.opentelemetry.io/otel/exporters/stdout/stdoutmetric`. (#7492)
- Improve the concurrent performance of `HistogramReservoir` in
`go.opentelemetry.io/otel/sdk/metric/exemplar` by 4x. (#7443)
- Improve performance of concurrent synchronous gauge measurements in
`go.opentelemetry.io/otel/sdk/metric`. (#7478)
- Improve performance of concurrent exponential histogram measurements
in `go.opentelemetry.io/otel/sdk/metric`. (#7702)
- Improve the concurrent performance of `FixedSizeReservoir` in
`go.opentelemetry.io/otel/sdk/metric/exemplar`. (#7447)
- The `rpc.grpc.status_code` attribute in the experimental metrics
emitted from
`go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc` is
replaced with the `rpc.response.status_code` attribute to align with the
semantic conventions. (#7854)
- The `rpc.grpc.status_code` attribute in the experimental metrics
emitted from
`go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc` is
replaced with the `rpc.response.status_code` attribute to align with the
semantic conventions. (#7854)

### Fixed

- Fix bad log message when key-value pairs are dropped because of key
duplication in `go.opentelemetry.io/otel/sdk/log`. (#7662)
- Fix `DroppedAttributes` on `Record` in
`go.opentelemetry.io/otel/sdk/log` to not count the non-attribute
key-value pairs dropped because of key duplication. (#7662)
- Fix `SetAttributes` on `Record` in `go.opentelemetry.io/otel/sdk/log`
to not log that attributes are dropped when they are actually not
dropped. (#7662)
- `WithHostID` detector in `go.opentelemetry.io/otel/sdk/resource` to
use full path for `ioreg` command on Darwin (macOS). (#7818)
- Fix missing `request.GetBody` in
`go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp` to
correctly handle HTTP2 GOAWAY frame. (#7794)

### Deprecated

- Deprecate `go.opentelemetry.io/otel/exporters/zipkin`. For more
information, see the [OTel blog post deprecating the Zipkin
exporter](https://opentelemetry.io/blog/2025/deprecating-zipkin-exporters/).
(#7670)

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sdk/log: Bad log message when key-value pairs are dropped because of key duplication

3 participants