Switch to new metric names in logs and OTLP export #1653

sahityadg · 2025-10-14T08:59:25Z

This PR updates metric names to follow consistent naming patterns for OTLP export. However, the logs are still not fully updated to use the new convention. This also adds integration tests for metrics validation

TODO: Remove all TODO messages in desc. For PR description guidance, review https://github.com/awslabs/mountpoint-s3/blob/main/doc/CONTRIBUTING.md#pull-request-title-and-description.

TODO: What changed and why?

Does this change impact existing behavior?

Yes, it changes metrics names in logs

Does this change need a changelog entry? Does it require a version change?

Yes, it require an update about metric names.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

Signed-off-by: Sahitya Damera <sahityad@amazon.com>

…ion tracking Signed-off-by: Sahitya Damera <sahityad@amazon.com>

Signed-off-by: Sahitya Damera <sahityad@amazon.com>

This adds TestRecorder utility for metrics validation in tests. This also updates tracing integation for metrics collection in all our tests. Signed-off-by: Sahitya Damera <sahityad@amazon.com>

Signed-off-by: Sahitya Damera <sahityad@amazon.com>

muddyfish · 2025-10-14T09:34:43Z

mountpoint-s3-fs/src/metrics.rs

    use opentelemetry_sdk::metrics::data::{AggregatedMetrics, MetricData, ResourceMetrics};
    use opentelemetry_sdk::metrics::in_memory_exporter::InMemoryMetricExporter;
    use opentelemetry_sdk::metrics::{PeriodicReader, SdkMeterProvider};
+    use std::sync::Arc;


Nit: No newline

muddyfish · 2025-10-14T11:01:52Z

mountpoint-s3-fs/tests/common/metrics.rs

+                }
+
+                // Every expected label must be in the actual labels
+                labels.iter().all(|(k, v)| actual_labels.contains(&(*k, *v)))


Nit: O(n^2) here though shouldn't matter due to it being only a test

muddyfish · 2025-10-14T13:15:37Z

mountpoint-s3-fs/tests/common/metrics.rs

+pub fn get_metric(recorder: &TestRecorder, name: &str, labels: &[(&str, &str)]) -> Arc<Metric> {
+    recorder
+        .get(name, labels)
+        .unwrap_or_else(|| panic!("Expected metric '{name}' with labels {labels:?} to exist"))


Use expect

muddyfish · 2025-10-14T13:17:41Z

mountpoint-s3-fs/tests/fuse_tests/metrics_test.rs

+    fn get_metric(recorder: &TestRecorder, name: &str, labels: &[(&str, &str)]) -> Arc<Metric> {
+        recorder
+            .get(name, labels)
+            .unwrap_or_else(|| panic!("Expected metric '{name}' with labels {labels:?} to exist"))


Use expect

muddyfish · 2025-10-14T13:19:47Z

mountpoint-s3-fs/tests/fuse_tests/metrics_test.rs

+        std::thread::sleep(Duration::from_millis(100));
+
+        let read_io_size = get_histogram(&context.recorder, FUSE_IO_SIZE, &[(ATTR_FUSE_REQUEST, "read")]);
+        assert!(read_io_size.contains(&1024.0));


Is this assertion really what we want to verify?

dannycjones

I haven't reviewed the test changes in detail nor the previous metric changes, but overall looks good

dannycjones · 2025-10-14T13:26:03Z

mountpoint-s3-client/src/metrics.rs

We already said that logged metrics aren't stable in docs, but I think it would be worth adding a note to the changelog to cover how we expect the metrics to change. (Any log line format change, metric name changes, etc..)

dannycjones · 2025-10-14T13:29:26Z

mountpoint-s3-fs/src/metrics.rs

            if last_mem != process.memory() {
-                metrics::gauge!("process.memory_usage").set(process.memory() as f64);
+                metrics::gauge!(PROCESS_MEMORY_USAGE).set(process.memory() as f64);
                metrics::gauge!("system.available_memory").set(sys.available_memory() as f64);


Why not also for system memory now?

dannycjones · 2025-10-14T13:33:59Z

mountpoint-s3-fs/tests/fuse_tests/metrics_test.rs

+use std::sync::Once;
+use std::time::Duration;
+
+static RECORDER: OnceCell<TestRecorder> = OnceCell::new();


I think we can replace all of the logic for RECORDER with std's LazyCell, or I'm assuming it'll actually need std's LazyLock.

https://doc.rust-lang.org/std/sync/struct.LazyLock.html

This should avoid taking a dependency on once_cell crate.

sahityadg requested a deployment to PR integration tests October 14, 2025 08:59 — with GitHub Actions Waiting

sahityadg force-pushed the otlp-feature branch from 3039320 to 64b17a6 Compare October 14, 2025 09:03

sahityadg temporarily deployed to PR integration tests October 14, 2025 09:03 — with GitHub Actions Inactive

sahityadg added 5 commits October 14, 2025 10:19

Rename S3_REQUEST_FAILURE metric to S3_REQUEST_ERRORS

bd576f4

Signed-off-by: Sahitya Damera <sahityad@amazon.com>

Rename mountpoint-s3-fs metrics for OTLP export

2b8df35

Signed-off-by: Sahitya Damera <sahityad@amazon.com>

Switch FUSE idle threads from gauge to histogram for better distribut…

3f60655

…ion tracking Signed-off-by: Sahitya Damera <sahityad@amazon.com>

Rename prefetch out of order metric for OTLP export

58d4c38

Signed-off-by: Sahitya Damera <sahityad@amazon.com>

Add integration tests for metrics verification

f5b5176

This adds TestRecorder utility for metrics validation in tests. This also updates tracing integation for metrics collection in all our tests. Signed-off-by: Sahitya Damera <sahityad@amazon.com>

sahityadg force-pushed the otlp-feature branch from 64b17a6 to ee07554 Compare October 14, 2025 09:27

sahityadg temporarily deployed to PR integration tests October 14, 2025 09:28 — with GitHub Actions Inactive

sahityadg force-pushed the otlp-feature branch from ee07554 to f5b5176 Compare October 14, 2025 09:30

sahityadg requested a deployment to PR integration tests October 14, 2025 09:30 — with GitHub Actions Waiting

sahityadg temporarily deployed to PR integration tests October 14, 2025 10:44 — with GitHub Actions Inactive

sahityadg requested a deployment to PR integration tests October 14, 2025 11:03 — with GitHub Actions Waiting

sahityadg force-pushed the otlp-feature branch from 77027fd to 21bbbfd Compare October 14, 2025 11:06

sahityadg temporarily deployed to PR integration tests October 14, 2025 11:06 — with GitHub Actions Inactive

sahityadg temporarily deployed to PR integration tests October 14, 2025 12:02 — with GitHub Actions Inactive

Add delays before verifying metrics to make them less flaky

fb4ca11

Signed-off-by: Sahitya Damera <sahityad@amazon.com>

sahityadg force-pushed the otlp-feature branch from 249842b to fb4ca11 Compare October 14, 2025 12:13

sahityadg had a problem deploying to PR integration tests October 14, 2025 12:13 — with GitHub Actions Failure

sahityadg temporarily deployed to PR integration tests October 14, 2025 12:14 — with GitHub Actions Inactive

sahityadg requested review from dannycjones, muddyfish and passaro October 14, 2025 12:33

muddyfish reviewed Oct 14, 2025

View reviewed changes

sahityadg marked this pull request as ready for review October 14, 2025 13:29

dannycjones reviewed Oct 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Switch to new metric names in logs and OTLP export #1653

Switch to new metric names in logs and OTLP export #1653

sahityadg commented Oct 14, 2025 •

edited

Loading

Uh oh!

muddyfish Oct 14, 2025

Uh oh!

muddyfish Oct 14, 2025

Uh oh!

muddyfish Oct 14, 2025

Uh oh!

muddyfish Oct 14, 2025

Uh oh!

muddyfish Oct 14, 2025

Uh oh!

dannycjones left a comment

Uh oh!

dannycjones Oct 14, 2025

Uh oh!

dannycjones Oct 14, 2025

Uh oh!

dannycjones Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Switch to new metric names in logs and OTLP export #1653

Are you sure you want to change the base?

Switch to new metric names in logs and OTLP export #1653

Conversation

sahityadg commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Does this change impact existing behavior?

Does this change need a changelog entry? Does it require a version change?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannycjones left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sahityadg commented Oct 14, 2025 •

edited

Loading