feat: add configurable telemetry support #36
Conversation
Mesa DescriptionSummary
Resolves MES-717 Config example[telemetry]
vendor = true
collector-url = "https://my-collector.example.com"Both Remaining workGrafana Alloy deployment at Test plan
Description generated by Mesa. Update settings |
|
LGTM, but needs testing |
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of 0453e34...491ab6d
Analysis
-
Telemetry initialization happens up front for every execution path, which could impact performance in paths that don't need tracing capabilities.
-
The exporter lifecycle is tightly coupled to process lifecycle (initialization, daemonization, shutdown), which may create issues with fork-safety and accurate success reporting.
-
Early failures before config loading fall back to
eprintln!, creating inconsistent command-line user experience depending on whether tracing was initialized. -
System correctness now depends on config validation and process lifecycle management rather than cargo features, introducing new potential points of failure.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
5 files reviewed | 1 comments | Edit Agent Settings • Read Docs
| std::process::exit(1); | ||
| } | ||
|
|
||
| let trc_handle = Trc::default() |
There was a problem hiding this comment.
Because tracing (and therefore the OTLP exporters) is initialized here before we know whether --daemonize is requested, the process forks after with_telemetry(..).init() has already built the BatchSpanProcessor threads (see Trc::build_otel_provider). Forking a multi‑threaded process is not fork‑safe: the daemonized child only contains the main thread, so the exporter worker threads are gone and queued spans will never flush when the daemon is running with telemetry enabled. To avoid silently breaking telemetry for daemonized runs, the OTLP provider needs to be instantiated only after the daemon fork (or re‑initialized inside the child) instead of before daemonize::Daemonize::start().
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: mesa-dot-dev/gitfs#36
File: src/main.rs#L69
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Because tracing (and therefore the OTLP exporters) is initialized here before we know whether `--daemonize` is requested, the process forks *after* `with_telemetry(..).init()` has already built the `BatchSpanProcessor` threads (see `Trc::build_otel_provider`). Forking a multi‑threaded process is not fork‑safe: the daemonized child only contains the main thread, so the exporter worker threads are gone and queued spans will never flush when the daemon is running with telemetry enabled. To avoid silently breaking telemetry for daemonized runs, the OTLP provider needs to be instantiated only after the daemon fork (or re‑initialized inside the child) instead of before `daemonize::Daemonize::start()`.
1 participant
Summary
[telemetry]config block withvendor(bool, default false) andcollector-url(optional string) for configuring OpenTelemetry trace export__otlp_exportfeature flag) so telemetry is runtime-configurableTrctracing builder to accept telemetry config and create per-endpoint OTLP batch exportersmain.rsto load config before tracing init so telemetry settings are availableResolves MES-717
Config example
Both
vendorandcollector-urlcan be active simultaneously — each gets its own independent OTLP batch exporter.Remaining work
Grafana Alloy deployment at
telemetry.priv.mesa.devis tracked separately as infrastructure work.Test plan
TelemetryConfigdeserialization andendpoints()logiccargo clippy --all-targets --all-features -- -D warningspassesvendor = truein config, verify OTLP exporter initializes (check "Telemetry export enabled" log)