8000 [SDK] Fixes duration overflow by owent · Pull Request #3529 · open-telemetry/opentelemetry-cpp · GitHub
[go: up one dir, main page]
Skip to content

[SDK] Fixes duration overflow #3529

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 10, 2025
Merged

[SDK] Fixes duration overflow #3529

merged 5 commits into from
Jul 10, 2025

Conversation

owent
Copy link
Member
@owent owent commented Jul 10, 2025

Fixes #3528

Changes

  • We do not use nanoseconds in background codes now, and duration conversion can be removed now.
  • Only convert duration from lager ratio types to small ratio types to avoid overflow.
  • These codes are tested in my own repo, which only have a small set of tests, turn on UBSAN tests may cause more warnings, and maybe we can add it in a standalone PR.

8000 For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@Copilot Copilot AI review requested due to automatic review settings July 10, 2025 03:13
@owent owent requested a review from a team as a code owner July 10, 2025 03:13
@netlify Netlify
Copy link
netlify bot commented Jul 10, 2025 •
edited
Loading

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit 20089a4
🔍 Latest deploy log https://app.netlify.com/projects/opentelemetry-cpp-api-docs/deploys/686feb74d7962b00086dc8d3

Copilot
Copilot AI reviewed Jul 10, 2025
View reviewed changes
Copy link
@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes unnecessary nanosecond conversions to prevent overflow and simplifies timeout handling by working directly with std::chrono::microseconds.

  • Eliminates casting to nanoseconds and unused overflow logic.
  • Updates ForceFlush and Shutdown to use microseconds consistently.
  • Adjusts expiration-time calculations accordingly.
Comments suppressed due to low confidence (2)

sdk/src/logs/multi_log_record_processor.cc:77

  • Consider adding unit tests for edge cases where timeout approaches system_clock::time_point::max to verify the new overflow logic under UBSAN.
bool MultiLogRecordProcessor::ForceFlush(std::chrono::microseconds timeout) noexcept

sdk/src/logs/multi_log_record_processor.cc:4

  • [nitpick] The header is added but no algorithms from it are used in this file. Consider removing it to keep includes minimal.
#include <algorithm>

@codecov Codecov
Copy link
codecov bot commented Jul 10, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.98%. Comparing base (4ad3b8b) to head (20089a4).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
sdk/src/logs/multi_log_record_processor.cc 60.00% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3529      +/-   ##
==========================================
+ Coverage   89.96%   89.98%   +0.02%     
==========================================
  Files         219      219              
  Lines        7051     7044       -7     
==========================================
- Hits         6343     6338       -5     
+ Misses        708      706       -2
Files with missing lines Coverage Δ
sdk/src/logs/multi_log_record_processor.cc 90.00% <60.00%> (+1.95%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

8000
lalitb
lalitb reviewed Jul 10, 2025
View reviewed changes
lalitb
lalitb reviewed Jul 10, 2025
View reviewed changes
lalitb
lalitb reviewed Jul 10, 2025
View reviewed changes
@marcalff marcalff mentioned this pull request Jul 10, 2025
Closed
@marcalff marcalff changed the title Fixes duration overflow [SDK] Fixes duration overflow Jul 10, 2025
@marcalff marcalff merged commit cafcfaa into open-telemetry:main Jul 10, 2025
70 checks passed
malkia added a commit to malkia/opentelemetry-cpp that referenced this pull request Jul 10, 2025
@malkia
Merge pull request #399 from open-telemetry/main
b99048f 
[SDK] Fixes duration overflow (open-telemetry#3529)
@BrewTestBot BrewTestBot mentioned this pull request Jul 11, 2025
Merged
@owent owent deleted the fixes_overflow_of_duration branch July 14, 2025 06:34
@gaborcsardi gaborcsardi mentioned this pull request Aug 1, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@lalitb lalitb lalitb approved these changes

@marcalff marcalff marcalff approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[UBSAN] Duration type overflow when enable asan with clang 20.1.3

3 participants

@owent @lalitb @marcalff
0