fix(platform): initialize logging before config resolution#892
Open
svvarom wants to merge 1 commit into
Open
Conversation
run_platform() resolved configuration before calling initialize_obs(), so anything logged during config loading fell back to Python's logging.lastResort handler: bare text on stderr, with everything below WARNING dropped. That put unparseable lines into the JSON log stream. Move observability init ahead of config resolution, and set LOG_FORMAT as an env var in the chart so the format is known without reading the config file. Also gate the interactive bind warning on a TTY check. Signed-off-by: Swarom Muley <smuley@nvidia.com>
Contributor
📝 WalkthroughWalkthroughStartup observability now initializes before configuration resolution, resolved logging settings are reapplied, Helm sets JSON logs, and bind-all warnings are limited to interactive terminals. ChangesObservability and operational logging
Sequence Diagram(s)sequenceDiagram
participant run_platform
participant Observability
participant Configuration
participant StructuredLogging
run_platform->>Observability: initialize_obs and setup_global_instrumentations
run_platform->>Configuration: resolve_run_configuration and apply_run_environment
run_platform->>StructuredLogging: initialize_logging when resolved settings differ
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nmp_platform_runner/tests/test_run.py (1)
99-142: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the reinitialization path and complete startup ordering.
This test only records
initialize_obsandresolve_run_configuration, so it would pass even ifsetup_global_instrumentationsran first or the newinitialize_logging()branch regressed. Add a case with differing environment/config log settings and assert reinitialization occurs before theresolve_configphase marker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nmp_platform_runner/tests/test_run.py` around lines 99 - 142, The test around run_platform must cover logging reinitialization and the complete startup order, not only initialize_obs versus resolve_run_configuration. Extend test_run_platform_initializes_logging_before_resolving_config to use differing environment/config logging settings, record initialize_logging, setup_global_instrumentations, and the resolve_config phase, then assert reinitialization occurs before resolve_run_configuration and the config-resolution marker, while preserving the existing initialization-order assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nmp_platform_runner/src/nmp/platform_runner/run.py`:
- Around line 97-108: Move _startup_phase("resolve_config", t0) in the run
configuration flow to after the service_config comparison and any
initialize_logging() call, while preserving the existing timing start and
logging reinitialization behavior.
---
Nitpick comments:
In `@packages/nmp_platform_runner/tests/test_run.py`:
- Around line 99-142: The test around run_platform must cover logging
reinitialization and the complete startup order, not only initialize_obs versus
resolve_run_configuration. Extend
test_run_platform_initializes_logging_before_resolving_config to use differing
environment/config logging settings, record initialize_logging,
setup_global_instrumentations, and the resolve_config phase, then assert
reinitialization occurs before resolve_run_configuration and the
config-resolution marker, while preserving the existing initialization-order
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 50896d29-ef07-41d8-8719-71c13093ef9a
📒 Files selected for processing (4)
k8s/helm/values.yamlpackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/services/cli.pypackages/nmp_platform_runner/src/nmp/platform_runner/run.pypackages/nmp_platform_runner/tests/test_run.py
Comment on lines
97
to
+108
| t0 = time.perf_counter() | ||
| resolved = resolve_run_configuration(config) | ||
| apply_run_environment(resolved) | ||
| _startup_phase("resolve_config", t0) | ||
|
|
||
| # The config file may still override the environment (local / CLI use), so re-initialize | ||
| # logging when it disagrees with what the environment already configured. | ||
| service_config = get_common_service_config() | ||
| if otel_settings.log_format != service_config.log_format: | ||
| if (otel_settings.log_format, otel_settings.log_level) != (service_config.log_format, service_config.log_level): | ||
| otel_settings.log_format = service_config.log_format | ||
| if otel_settings.log_level != service_config.log_level: | ||
| otel_settings.log_level = service_config.log_level | ||
|
|
||
| t0 = time.perf_counter() | ||
| initialize_obs(resource_attributes=get_platform_resource_attributes()) | ||
| setup_global_instrumentations() | ||
| _startup_phase("observability_init", t0) | ||
| initialize_logging() |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reinitialize logging before recording the resolve phase.
_startup_phase("resolve_config", t0) runs before the config-driven initialize_logging(). When local or CLI configuration changes log_format or log_level, this startup record can still use the environment-based logger, leaving mixed-format startup output.
Move the phase timing after the comparison and reinitialization:
Proposed ordering
resolved = resolve_run_configuration(config)
apply_run_environment(resolved)
-_startup_phase("resolve_config", t0)
service_config = get_common_service_config()
if (...) :
...
initialize_logging()
+_startup_phase("resolve_config", t0)📝 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.
Suggested change
| t0 = time.perf_counter() | |
| resolved = resolve_run_configuration(config) | |
| apply_run_environment(resolved) | |
| _startup_phase("resolve_config", t0) | |
| # The config file may still override the environment (local / CLI use), so re-initialize | |
| # logging when it disagrees with what the environment already configured. | |
| service_config = get_common_service_config() | |
| if otel_settings.log_format != service_config.log_format: | |
| if (otel_settings.log_format, otel_settings.log_level) != (service_config.log_format, service_config.log_level): | |
| otel_settings.log_format = service_config.log_format | |
| if otel_settings.log_level != service_config.log_level: | |
| otel_settings.log_level = service_config.log_level | |
| t0 = time.perf_counter() | |
| initialize_obs(resource_attributes=get_platform_resource_attributes()) | |
| setup_global_instrumentations() | |
| _startup_phase("observability_init", t0) | |
| initialize_logging() | |
| t0 = time.perf_counter() | |
| resolved = resolve_run_configuration(config) | |
| apply_run_environment(resolved) | |
| # The config file may still override the environment (local / CLI use), so re-initialize | |
| # logging when it disagrees with what the environment already configured. | |
| service_config = get_common_service_config() | |
| if (otel_settings.log_format, otel_settings.log_level) != (service_config.log_format, service_config.log_level): | |
| otel_settings.log_format = service_config.log_format | |
| otel_settings.log_level = service_config.log_level | |
| initialize_logging() | |
| _startup_phase("resolve_config", t0) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/nmp_platform_runner/src/nmp/platform_runner/run.py` around lines 97
- 108, Move _startup_phase("resolve_config", t0) in the run configuration flow
to after the service_config comparison and any initialize_logging() call, while
preserving the existing timing start and logging reinitialization behavior.
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Piping platform logs to
jqfails. Startup emits plain text before JSON logging is configured:Two separate causes:
run_platform()resolved configuration before callinginitialize_obs(). Anything logged in that window has no handler on the root logger, so Python falls back tologging.lastResort— bare text on stderr, and everything below WARNING silently dropped.[STARTUP] resolve_configwas being lost this way.typer.echo, not a log record at all.The format could not be known earlier because the chart only set
config.service.log_format: jsonin the mounted config file — and reading that file is exactly what logs.Changes
run.py— moveinitialize_obs()ahead of config resolution. Re-initialize logging afterward only if the config file disagrees with the environment (local / CLI use).values.yaml— setLOG_FORMAT: "json"under the existingtelemetry:env passthrough. BothOTELSettingsandCommonServiceConfigread the bareLOG_FORMAT, so logging can be configured with no config file. No template changes; the existingnemo-common.otel-envhelper already feeds both deployments.cli.py— gate the bind warning onsys.stderr.isatty(). In a pod, binding to0.0.0.0is the intended configuration.Verification
[STARTUP] resolve_confignow appears instead of being dropped.helm templaterendersLOG_FORMATinto both the api and core-controller deployments.nmp_platform_runner112 passed,test_common_config.py+test_otel.py45 passed. New test assertsinitialize_obsruns beforeresolve_run_configuration.ruff check/ruff formatclean.Not included
Docker is not availablestill logs twice, now as two JSON lines. Separate cause:global_settings_to_service_configconstructs the config model twice (once for defaults + env, once with file values merged), and Pydantic runsvalidate_runtimeon each. Both constructions are load-bearing. Left for a follow-up.The vendored SDK copy of
cli.pystill has the ungated warning; it picks up the change on the nextmake update-sdk.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes