Skip to content

Conversation

@scottgerring
Copy link
Member

@scottgerring scottgerring commented Feb 25, 2025

This PR follows on from #2705, introducing a workflow that publishes a criterion regression comparison between main and the current branch.

It cannot be merged and the build will not succeed until #2705 is in place.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@codecov
Copy link

codecov bot commented Feb 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.3%. Comparing base (7954252) to head (e9b77ca).
Report is 2 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2706   +/-   ##
=====================================
  Coverage   79.3%   79.3%           
=====================================
  Files        123     123           
  Lines      22670   22670           
=====================================
  Hits       17986   17986           
  Misses      4684    4684           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cijothomas
Copy link
Member

Closes #251

@scottgerring scottgerring mentioned this pull request Feb 26, 2025
4 tasks
@scottgerring scottgerring marked this pull request as ready for review February 26, 2025 07:21
@scottgerring scottgerring requested a review from a team as a code owner February 26, 2025 07:21
@scottgerring
Copy link
Member Author

@cijothomas this is good to go.

I'm not sure if the permissions are going to work for this to submit to the PR feedback from forks. I am hopeful that once the workflow is in main it will, but we'll see. If it doesn't I'll spend some more time looking at if we can set the perms up differently to achieve this (perhaps with repository settings) after this goes in.

Regardless, you always see the performance comparison in the job run:

https://github.com/open-telemetry/opentelemetry-rust/actions/runs/13538282948/job/37833792773?pr=2706

@cijothomas cijothomas merged commit 261ac75 into open-telemetry:main Feb 26, 2025
22 of 23 checks passed
@cijothomas
Copy link
Member

@cijothomas this is good to go.

I'm not sure if the permissions are going to work for this to submit to the PR feedback from forks. I am hopeful that once the workflow is in main it will, but we'll see. If it doesn't I'll spend some more time looking at if we can set the perms up differently to achieve this (perhaps with repository settings) after this goes in.

Regardless, you always see the performance comparison in the job run:

https://github.com/open-telemetry/opentelemetry-rust/actions/runs/13538282948/job/37833792773?pr=2706

merged. let us see !

@scottgerring
Copy link
Member Author

@cijothomas benchmark_collect_histogram intermittently fails. I don't want to introduce a CI check that breaks the build. I'd rather comment out this assertion for now with a TODO so we can get this in. What do you think?

thread 'main' panicked at opentelemetry-sdk/benches/metric.rs:348:36:
index out of bounds: the len is 0 but the index is 0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fn benchmark_collect_histogram(b: &mut Bencher, n: usize) {
    let r = SharedReader(Arc::new(ManualReader::default()));
    let mtr = SdkMeterProvider::builder()
        .with_reader(r.clone())
        .build()
        .meter("sdk/metric/bench/histogram");

    for i in 0..n {
        let h = mtr.u64_histogram(format!("fake_data_{i}")).build();
        h.record(1, &[]);
    }

    let mut rm = ResourceMetrics {
        resource: Resource::builder_empty().build(),
        scope_metrics: Vec::new(),
    };

    b.iter(|| {
        let _ = r.collect(&mut rm);
        assert_eq!(rm.scope_metrics[0].metrics.len(), n); // Fails here
    })

@cijothomas
Copy link
Member

@cijothomas benchmark_collect_histogram intermittently fails. I don't want to introduce a CI check that breaks the build. I'd rather comment out this assertion for now with a TODO so we can get this in. What do you think?

thread 'main' panicked at opentelemetry-sdk/benches/metric.rs:348:36:
index out of bounds: the len is 0 but the index is 0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fn benchmark_collect_histogram(b: &mut Bencher, n: usize) {
   let r = SharedReader(Arc::new(ManualReader::default()));
   let mtr = SdkMeterProvider::builder()
       .with_reader(r.clone())
       .build()
       .meter("sdk/metric/bench/histogram");

   for i in 0..n {
       let h = mtr.u64_histogram(format!("fake_data_{i}")).build();
       h.record(1, &[]);
   }

   let mut rm = ResourceMetrics {
       resource: Resource::builder_empty().build(),
       scope_metrics: Vec::new(),
   };

   b.iter(|| {
       let _ = r.collect(&mut rm);
       assert_eq!(rm.scope_metrics[0].metrics.len(), n); // Fails here
   })

yes, we can comment it out with TODO, and fix that separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants