Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests to validate spatial aggregation #1416

Merged

Conversation

cijothomas
Copy link
Member

Added tests to validate SDK is doing "spatial aggregation" correctly. That term is no longer used explicitly in the spec, but the expected behavior is not currently implemented. Based on my experience, many implementations had/have this bug.

eg: .NET had this bug from day1 and not fixed as of today. https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/test/OpenTelemetry.Tests/Metrics/MetricApiTestsBase.cs#L883-L886

Related discussions on this topic:
open-telemetry/opentelemetry-specification#2905
open-telemetry/opentelemetry-specification#1874

The tests are ignored now (they won't pass today!), but I think its worth while to have them, while we work on Metrics SDK, to help us keep in mind some edge scenarios.

I'll dig more into fixing these. The shared discussions above only affected Observable instruments, but we are failing for Non-observable as well here, so there are likely more changes required.

@cijothomas cijothomas requested a review from a team November 30, 2023 18:01
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Attention: 151 lines in your changes are missing coverage. Please review.

Comparison is base (3594247) 58.4% compared to head (5132858) 57.9%.

Files Patch % Lines
opentelemetry-sdk/src/metrics/mod.rs 0.0% 151 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1416     +/-   ##
=======================================
- Coverage   58.4%   57.9%   -0.5%     
=======================================
  Files        146     146             
  Lines      18346   18497    +151     
=======================================
  Hits       10723   10723             
- Misses      7623    7774    +151     

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

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks. We have the same problem in otel-cpp for the Observable counter. I think synchronous works fine.

opentelemetry-sdk/src/metrics/mod.rs Show resolved Hide resolved
opentelemetry-sdk/src/metrics/mod.rs Show resolved Hide resolved
@cijothomas
Copy link
Member Author

Merging. I'll look at fixing this for sync instruments.

Fixing for Observable is likely harder - haven't been able to solve in .NET, C++ yet, unfortunately :-( Hopefully Rust can fix that.)

@cijothomas cijothomas merged commit 0862af1 into open-telemetry:main Nov 30, 2023
14 of 15 checks passed
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