feat: follow opentelemetry semantic conventions#633
Conversation
WalkthroughThis PR removes CLI-provided OTLP parameters and the OpenTelemetry config struct, switches observability wiring to workspace/default OTEL values, adds the Changes
Sequence Diagram(s)(Skipped — changes are refactor/configuration-focused and do not introduce a new multi-component control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
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 |
|
@nemo83 I recall you mentioned at some point that |
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
jeluard
force-pushed
the
jeluard/opentelemetry-semantic-conventions
branch
from
cbca19f to
b9f6af5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/amaru/src/bin/amaru/main.rs (1)
124-134: Startupinfo!likely gets dropped: log happens before tracing subscriber init.Right now
info!(...)(Line 124) runs beforesetup_observability(...)(Line 130) callssubscriber.init(), so you may lose the “Started with global arguments” line.Proposed fix
- info!( - with_open_telemetry = args.with_open_telemetry, - with_json_traces = args.with_json_traces, - "Started with global arguments" - ); - let (metrics, teardown) = setup_observability( args.with_open_telemetry, args.with_json_traces, ); + + info!( + with_open_telemetry = args.with_open_telemetry, + with_json_traces = args.with_json_traces, + "Started with global arguments" + );
🤖 Fix all issues with AI agents
In @crates/amaru/src/observability.rs:
- Around line 241-243: The metric exporter is hardcoding the endpoint via
opentelemetry_otlp::MetricExporter::builder().with_endpoint(DEFAULT_OTLP_METRIC_URL)
which overrides OTEL_EXPORTER_OTLP_METRICS_ENDPOINT; remove the explicit
.with_endpoint(...) call from the metric_exporter builder so it behaves like the
span exporter (which does not set an endpoint) and therefore respects standard
OTEL env vars, or alternatively change the code to read the OTEL env var and
only call .with_endpoint(...) when that var is absent.
🧹 Nitpick comments (3)
monitoring/README.md (1)
361-374: Documentation's on point, but polish these minor bits.Ace work updating the docs to reflect the standard OTEL env vars! This aligns perfectly with the code changes. The explanation about using OTLP/gRPC for spans and OTLP/HTTP for metrics is solid context – like explaining why you chose the plasma rifle over the BFG in DOOM, yeah?
However, there are a few small nits to polish:
- Line 361: "recognizes" has a typo (extra space or wrong variant?)
- Line 372: The linter's right – "maximise" vs "maximize" should be consistent, and "3rd party" should be "3rd-party"
- Line 374: Both links say "here" which isn't very descriptive – maybe "OpenTelemetry SDK environment variables" and "OTLP exporter specification" would be more helpful?
These are cosmetic, but worth tidying up for a professional finish.
✨ Suggested polish for documentation
-Amaru recognizes standard OpenTelemetry env variable for its configuration: +Amaru recognizes standard OpenTelemetry environment variables for its configuration: -This helps maximize compatibility with 3rd party tools receiving those data. +This helps maximize compatibility with 3rd-party tools receiving those data. -One can find more available env variables [here](https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/) and [here](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md). +One can find more available environment variables in the [OpenTelemetry SDK documentation](https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/) and [OTLP exporter specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md).crates/amaru/src/bin/amaru/main.rs (2)
147-150: Prefertracing::error!overeprintln!for teardown failures (unless you want it outside telemetry).Since tracing is initialized, logging teardown failures via tracing keeps everything in one place.
104-113: Mind the env-var storytelling here, mate—OTEL doesn't quite work the way the concern frames it.Quick clarification: OpenTelemetry's standard doesn't have a boolean "enable OTLP" toggle like
AMARU_WITH_OPEN_TELEMETRY. Instead, it usesOTEL_TRACES_EXPORTER=otlpornoneto flip the exporter on/off per signal. SettingOTEL_EXPORTER_OTLP_ENDPOINTalone won't magically enable export—you still need the exporter selected viaOTEL_TRACES_EXPORTER.That said, the UX friction is real: if someone's only familiar with OTEL's standard variables and sets
OTEL_EXPORTER_OTLP_ENDPOINT, they'll hit a wall unless they also know aboutAMARU_WITH_OPEN_TELEMETRY.If you want to lean full OTEL-standard here, consider deriving the toggles from
OTEL_TRACES_EXPORTER/OTEL_METRICS_EXPORTER(or accept both old and new for backwards compatibility). Otherwise, a note in docs explaining the custom toggles layer on top of OTEL config would save future maintainers a head-scratch moment.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/amaru/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlcrates/amaru/Cargo.tomlcrates/amaru/src/bin/amaru/main.rscrates/amaru/src/bin/ledger/main.rscrates/amaru/src/observability.rsdocker/start_amaru.shengineering-decision-records/007-observability.mdmonitoring/README.md
💤 Files with no reviewable changes (2)
- engineering-decision-records/007-observability.md
- crates/amaru/src/bin/ledger/main.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T12:28:24.027Z
Learnt from: etorreborre
Repo: pragma-org/amaru PR: 372
File: simulation/amaru-sim/src/simulator/mod.rs:410-412
Timestamp: 2025-08-12T12:28:24.027Z
Learning: In the Amaru project, panic statements are acceptable in simulation/test code (like amaru-sim crate) as they help identify configuration issues quickly during development, rather than needing proper error handling like production code.
Applied to files:
docker/start_amaru.sh
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru/src/bin/amaru/main.rscrates/amaru/src/observability.rs
🧬 Code graph analysis (1)
crates/amaru/src/bin/amaru/main.rs (1)
crates/amaru/src/observability.rs (1)
setup_observability(320-351)
🪛 LanguageTool
monitoring/README.md
[uncategorized] ~372-~372: Do not mix variants of the same word (‘maximize’ and ‘maximise’) within a single text.
Context: ...ans * OTLP/HTTP for metrics This helps maximize compatibility with 3rd party tools rece...
(EN_WORD_COHERENCY)
[grammar] ~372-~372: Use a hyphen to join words.
Context: ...is helps maximize compatibility with 3rd party tools receiving those data. One c...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
monitoring/README.md
374-374: Link text should be descriptive
(MD059, descriptive-link-text)
374-374: Link text should be descriptive
(MD059, descriptive-link-text)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
crates/amaru/src/observability.rs (2)
18-18: Ripper! Using semantic conventions like a pro.Bonzer choice importing
SERVICE_NAMEfrom the semantic conventions crate. This is the way to go, mate – keeps everything aligned with the OTEL spec rather than rolling your own constants. Like choosing the Master Sword over a rusty stick in Zelda, innit?
320-351: Clean as a whistle! Simplified like a speedrun route.Love how you've streamlined
setup_observabilityto just take the two boolean flags. The refactor removes all that config plumbing and makes the API much more straightforward. It's like going from a convoluted Dark Souls boss fight to a clean one-shot, yeah?Much easier for callers to reason about now.
crates/amaru/Cargo.toml (1)
43-43: Sorted! Dependency wired up properly.Adding
opentelemetry-semantic-conventionsto match the workspace definition. No dramas here – standard dependency management, innit? Everything's aligned with the root Cargo.toml.docker/start_amaru.sh (1)
28-28: Good on ya! Cleaning up the gasket remnants.Removing the
gasket=errortarget from the logging config – makes sense as part of the broader cleanup mentioned in the commits. Keeping things focused on amaru-specific logging. It's like tidying up after a house party, getting rid of the empty tinnies, yeah?Cargo.toml (1)
47-47: Version is solid – 0.31.0 exists and is properly compatible with your other OTEL deps.Mate, you've nailed it here.
opentelemetry-semantic-conventions = "0.31.0"is absolutely the right call – it's confirmed live on crates.io and plays nice withopentelemetry 0.31.0. Keeping all your OTEL crates locked to the same version is the way. It's like making sure everyone in your party is on the same quest difficulty – no one's gonna get left behind wondering why their builds don't line up. Rock on!crates/amaru/src/bin/amaru/main.rs (1)
15-15: Nice cleanup: import now matches the simplified observability API.
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
|
|
||
| #[arg(long, value_name = "STRING", env("AMARU_OTLP_SERVICE_NAME"), default_value_t = DEFAULT_OTLP_SERVICE_NAME.to_string() | ||
| )] | ||
| otlp_service_name: String, | ||
|
|
||
| #[arg(long, value_name = "URL", env("AMARU_OTLP_SPAN_URL"), default_value_t = DEFAULT_OTLP_SPAN_URL.to_string() | ||
| )] | ||
| otlp_span_url: String, | ||
|
|
||
| #[arg(long, value_name = "URL", env("AMARU_OTLP_METRIC_URL"), default_value_t = DEFAULT_OTLP_METRIC_URL.to_string() | ||
| )] | ||
| otlp_metric_url: String, |
There was a problem hiding this comment.
convention over configuration. LEt's go!
|
Love it! Thank you! |
Codecov Report❌ Patch coverage is
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
3 participants
Pave the way to make sure OpenTelemetry conventions are followed. Also relies on default env variable and not custom amaru ones for OTEL.
Summary by CodeRabbit
Documentation
Refactor
Chores
Other
✏️ Tip: You can customize this high-level summary in your review settings.