feat: add configurable log processing parameters to telemetry system#39
feat: add configurable log processing parameters to telemetry system#39andersonleal merged 3 commits intomainfrom
Conversation
- Introduced `logsFlushIntervalMs` and `logsBatchSize` options in the `OtelConfig` interface for customizable log processing behavior. - Updated the Node.js and Python SDKs to utilize these new configuration options, allowing users to set log flush intervals and batch sizes via environment variables or directly in the configuration. - Enhanced the Rust SDK to support similar log processing configurations, ensuring consistency across all SDKs. - Added tests to verify default values and explicit configurations for log processing parameters.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds configurable log batching to the OpenTelemetry telemetry system across TypeScript, Python, and Rust implementations. New configuration fields ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/node/iii/src/telemetry-system/index.ts`:
- Around line 170-188: Run Prettier on the file to fix formatting
inconsistencies: run the suggested command in the package (cd packages/node/iii
&& npx prettier --write src/telemetry-system/index.ts) so the block around
logsScheduledDelayMillis, logsMaxExportBatchSize, parseNumberEnv,
parseIntegerEnv, LoggerProvider and BatchLogRecordProcessor is formatted
according to project style; alternatively, apply the project's Prettier rules to
reflow line breaks and spacing in that section and then commit the updated file.
In `@packages/node/iii/src/telemetry-system/utils.ts`:
- Around line 12-17: The block containing parseIntegerEnv (which calls
parseNumberEnv) is not formatter-compliant; run the project formatter on this
file or reformat the parseIntegerEnv function to match the repository style
(e.g., spacing, semicolons/newlines) so CI passes—locate the parseIntegerEnv
function and apply the project's formatter (or run the project's format
script/Prettier/ESLint --fix) to update the formatting.
- Around line 4-8: parseNumberEnv currently coerces an empty string ("") to 0
via Number("") which can override defaults; update parseNumberEnv to treat
empty-string inputs as undefined before numeric coercion by returning undefined
if value === "" (in addition to value === undefined), then continue parsing with
Number(value) and validate against Number.isFinite(parsed) and parsed < minimum
as before; reference the function parseNumberEnv so reviewers can locate and
update its handling of empty-string env values.
In `@packages/python/iii/src/iii/telemetry.py`:
- Around line 206-212: The type ignore is placed on the closing parenthesis
instead of the actual mismatched argument; move the "# type: ignore[arg-type]"
to the log_exporter argument in the BatchLogRecordProcessor call so the type
checker ignores the EngineLogExporter vs LogRecordExporter incompatibility
(i.e., change the call to pass "log_exporter, # type: ignore[arg-type]" to the
BatchLogRecordProcessor inside log_provider.add_log_record_processor).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/rust/iii/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
packages/node/iii/src/telemetry-system/index.tspackages/node/iii/src/telemetry-system/types.tspackages/node/iii/src/telemetry-system/utils.tspackages/python/iii/src/iii/telemetry.pypackages/python/iii/src/iii/telemetry_types.pypackages/python/iii/tests/test_telemetry.pypackages/python/iii/tests/test_telemetry_types.pypackages/rust/iii/src/telemetry/mod.rspackages/rust/iii/src/telemetry/types.rs
| export function parseNumberEnv(value: string | undefined, minimum: number = 0): number | undefined { | ||
| if (value === undefined) return undefined | ||
| const parsed = Number(value) | ||
| if (!Number.isFinite(parsed) || parsed < minimum) return undefined | ||
| return parsed |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and examine its structure
find . -type f -name "utils.ts" | grep -i telemetryRepository: iii-hq/sdk
Length of output: 103
🏁 Script executed:
# Also check the overall structure of the packages/node directory
git ls-files packages/node/ | head -30Repository: iii-hq/sdk
Length of output: 1209
🏁 Script executed:
# Search for parseNumberEnv usage across the codebase
rg -l "parseNumberEnv" --type=tsRepository: iii-hq/sdk
Length of output: 149
🏁 Script executed:
# Read the utils.ts file to see the implementation
cat -n packages/node/iii/src/telemetry-system/utils.tsRepository: iii-hq/sdk
Length of output: 840
🏁 Script executed:
# Check the index.ts to see how parseNumberEnv is used
cat -n packages/node/iii/src/telemetry-system/index.tsRepository: iii-hq/sdk
Length of output: 11461
🏁 Script executed:
# Search for all usages of parseNumberEnv with context
rg -n "parseNumberEnv" -C5 --type=tsRepository: iii-hq/sdk
Length of output: 3660
🏁 Script executed:
# Search for test files related to this utility
find . -type f \( -name "*test*" -o -name "*spec*" \) -path "*/telemetry-system/*" | head -20Repository: iii-hq/sdk
Length of output: 36
🏁 Script executed:
# Search for test files in the entire packages/node/iii directory
find packages/node/iii -type f \( -name "*.test.ts" -o -name "*.spec.ts" \)Repository: iii-hq/sdk
Length of output: 581
🏁 Script executed:
# Search for any tests referencing parseNumberEnv or parseIntegerEnv
rg -n "(parseNumberEnv|parseIntegerEnv)" --type=ts -g "*test*" -g "*spec*"Repository: iii-hq/sdk
Length of output: 36
🏁 Script executed:
# Check DEFAULT_OTEL_CONFIG to understand the defaults
rg -n "DEFAULT_OTEL_CONFIG\|logsFlushIntervalMs\|logsBatchSize" --type=ts -B2 -A2Repository: iii-hq/sdk
Length of output: 36
🏁 Script executed:
# Check if there are any guards before the parseNumberEnv calls
rg -n "OTEL_LOGS_FLUSH_INTERVAL_MS\|OTEL_LOGS_BATCH_SIZE" --type=ts -B3 -A3Repository: iii-hq/sdk
Length of output: 36
🏁 Script executed:
# Look at the otel-defaults test file
cat -n packages/node/iii/tests/otel-defaults.test.tsRepository: iii-hq/sdk
Length of output: 9587
🏁 Script executed:
# Search for DEFAULT_OTEL_CONFIG definition
rg -n "DEFAULT_OTEL_CONFIG" --type=tsRepository: iii-hq/sdk
Length of output: 1386
🏁 Script executed:
# Search for logsFlushIntervalMs and logsBatchSize in types.ts
cat packages/node/iii/src/telemetry-system/types.tsRepository: iii-hq/sdk
Length of output: 3960
🏁 Script executed:
# Test JavaScript Number coercion behavior to confirm the claim
node -e "console.log('Number(\"\") =', Number('')); console.log('Number(\" \") =', Number(' ')); console.log('Number(\"0\") =', Number('0')); console.log('Number(\"abc\") =', Number('abc'))"Repository: iii-hq/sdk
Length of output: 122
Add empty-string validation before numeric coercion in parseNumberEnv.
Environment variables like OTEL_LOGS_FLUSH_INTERVAL_MS="" pass Number("") which coerces to 0, silently overriding defaults (e.g., logsFlushIntervalMs: 100, logsBatchSize: 1). This causes incorrect configuration—empty env vars should be treated as unset, not as zero.
Currently called on lines 172 and 176 of index.ts with no upstream guards.
Proposed fix
export function parseNumberEnv(value: string | undefined, minimum: number = 0): number | undefined {
if (value === undefined) return undefined
- const parsed = Number(value)
+ const normalized = value.trim()
+ if (normalized.length === 0) return undefined
+ const parsed = Number(normalized)
if (!Number.isFinite(parsed) || parsed < minimum) return undefined
return parsed
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function parseNumberEnv(value: string | undefined, minimum: number = 0): number | undefined { | |
| if (value === undefined) return undefined | |
| const parsed = Number(value) | |
| if (!Number.isFinite(parsed) || parsed < minimum) return undefined | |
| return parsed | |
| export function parseNumberEnv(value: string | undefined, minimum: number = 0): number | undefined { | |
| if (value === undefined) return undefined | |
| const normalized = value.trim() | |
| if (normalized.length === 0) return undefined | |
| const parsed = Number(normalized) | |
| if (!Number.isFinite(parsed) || parsed < minimum) return undefined | |
| return parsed | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/node/iii/src/telemetry-system/utils.ts` around lines 4 - 8,
parseNumberEnv currently coerces an empty string ("") to 0 via Number("") which
can override defaults; update parseNumberEnv to treat empty-string inputs as
undefined before numeric coercion by returning undefined if value === "" (in
addition to value === undefined), then continue parsing with Number(value) and
validate against Number.isFinite(parsed) and parsed < minimum as before;
reference the function parseNumberEnv so reviewers can locate and update its
handling of empty-string env values.
- Simplified the logger provider setup logic in the `init_otel` function for improved readability and maintainability. - Added a new `get_logger_provider` function to retrieve a clone of the logger provider, enhancing the SDK's logging capabilities. - Removed redundant code and ensured consistent handling of log processing parameters.
- Enhanced readability by adjusting the formatting of the logger provider initialization in the `initOtel` function. - Updated the `parseIntegerEnv` function to improve its parameter formatting for better clarity. - Added type casting for `log_exporter` in the Python telemetry module to ensure type safety.
2 participants
Summary
Type of change
Checklist
Additional context
developdoes not exist in this repository; PR is targeted tomain(default branch).OTEL_LOGS_FLUSH_INTERVAL_MSOTEL_LOGS_BATCH_SIZESummary by CodeRabbit
Release Notes