Skip to content

Conversation

@MPSAJID
Copy link
Contributor

@MPSAJID MPSAJID commented Jan 16, 2026

This PR extends test coverage for OTEL_BLRP_SCHEDULE_DELAY to include
additional invalid inputs such as negatives, zero, overflow,
and empty strings.

Follow-up to #3811.

@MPSAJID MPSAJID requested a review from a team as a code owner January 16, 2026 18:04
@lalitb
Copy link
Member

lalitb commented Jan 17, 2026

Seem we have caught a bug in the code here as CI is failing. Either ignore/comment the failing test adding a TODO, or fix the code (should be easy fix I guess).

.cc:183 Environment variable <OTEL_BLRP_SCHEDULE_DELAY> has an invalid value <0ms>, ignoring
sdk/test/logs/batch_log_record_processor_test.cc:431: Failure
Expected equality of these values:
  options.schedule_delay_millis
    Which is: 8-byte object <F9-AF 75-11 9E-04 00-00>
  std::chrono::milliseconds(1000)
    Which is: 8-byte object <E8-03 00-00 00-00 00-00>
Failed for case: overflow (value='999999999999999999999s')

@codecov
Copy link

codecov bot commented Jan 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.96%. Comparing base (7d41406) to head (9c9e9c5).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3812      +/-   ##
==========================================
+ Coverage   89.96%   89.96%   +0.01%     
==========================================
  Files         225      225              
  Lines        7167     7170       +3     
==========================================
+ Hits         6447     6450       +3     
  Misses        720      720              
Files with missing lines Coverage Δ
sdk/src/common/env_variables.cc 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MPSAJID
Copy link
Contributor Author

MPSAJID commented Jan 17, 2026

The test failure exposed a missing overflow check in GetTimeoutFromString.
I’ve added a guard to reject values that would overflow during numeric parsing, so such inputs are now consistently treated as invalid and fall back to defaults.

One clarification question: is a duration value of 0 intended to be valid?
Currently GetTimeoutFromString treats 0 as invalid, but for the unit-less case ("") a value of 0 would otherwise be accepted as seconds.
If 0 is meant to be allowed, I can update the tests (and logic if needed) accordingly; otherwise the current behavior and tests are consistent.

@open-telemetry open-telemetry deleted a comment from TheMenisFactor Jan 18, 2026
@open-telemetry open-telemetry deleted a comment from MPSAJID Jan 18, 2026
@open-telemetry open-telemetry deleted a comment from TheMenisFactor Jan 18, 2026
Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

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

LGTM after the format issued is fixed.

@MPSAJID MPSAJID force-pushed the logs-extend-invalid-schedule-delay-tests branch from db24eb6 to 9c9e9c5 Compare January 18, 2026 14:18
@MPSAJID
Copy link
Contributor Author

MPSAJID commented Jan 19, 2026

@lalitb @owent format issue is fixed.

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.

3 participants