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

Make MetricPoint reclaim an opt-in experimental feature #5052

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Nov 15, 2023

Related to #4486

Changes

  • Revert the default behavior of Metrics SDK to not reclaim Metric Points
  • Add opt-in support for MetricPoint reclaim by introducing the key OTEL_DOTNET_EXPERIMENTAL_METRICS_RECLAIM_UNUSED_METRIC_POINTS
  • Update unit tests to rely on in-memory configuration instead of environment variables to avoid collision when running concurrently

Merge requirement checklist

  • Appropriate CHANGELOG.md files updated for non-trivial changes

@utpilla utpilla requested a review from a team as a code owner November 15, 2023 00:23
@utpilla utpilla changed the title Make MetricPoint reclaim an opt-in experimental behavior Make MetricPoint reclaim an opt-in experimental feature Nov 15, 2023
Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #5052 (67ddb89) into main (e904994) will decrease coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 67ddb89 differs from pull request most recent head 91424a9. Consider uploading reports for the commit 91424a9 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5052      +/-   ##
==========================================
- Coverage   83.54%   83.53%   -0.02%     
==========================================
  Files         296      296              
  Lines       12481    12479       -2     
==========================================
- Hits        10427    10424       -3     
- Misses       2054     2055       +1     
Flag Coverage Δ
unittests 83.53% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/OpenTelemetry/Metrics/AggregatorStore.cs 78.53% <100.00%> (-1.33%) ⬇️
src/OpenTelemetry/Metrics/MeterProviderSdk.cs 89.97% <100.00%> (+0.02%) ⬆️
src/OpenTelemetry/Metrics/Metric.cs 96.49% <100.00%> (+0.03%) ⬆️
src/OpenTelemetry/Metrics/MetricPoint.cs 68.47% <100.00%> (ø)
src/OpenTelemetry/Metrics/MetricReaderExt.cs 87.06% <100.00%> (+0.22%) ⬆️

... and 7 files with indirect coverage changes

Copy link
Member

@vishweshbankwar vishweshbankwar left a comment

Choose a reason for hiding this comment

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

LGTM - We should document the experimental features here

[InlineData("FALSE", false)]
[InlineData("true", true)]
[InlineData("True", true)]
[InlineData("TRUE", true)]
Copy link
Member

Choose a reason for hiding this comment

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

nit: add null case

Comment on lines 67 to 73
using var meterProvider = Sdk.CreateMeterProviderBuilder()
.ConfigureServices(services =>
{
services.AddSingleton(this.configuration);
})
.AddMeter(meter.Name)
.AddInMemoryExporter(exportedItems)
Copy link
Member

Choose a reason for hiding this comment

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

No biggie if you don't want to do take this on, but seems like these lines are repeated a bajillion times in this file. Might be a good extract method opportunity.

Copy link
Member

Choose a reason for hiding this comment

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

Bajillion is an informal word that means a very large but unspecified number. It is not a real number, but a way of exaggerating or expressing a huge quantity. For example, one might say “I have a bajillion things to do today” to mean that they have a lot of tasks. The word bajillion is probably a blend of bazillion and jillion, which are also informal words for a large number12. There is no standard way to write bajillion in numerals, since it is not a precise value. However, some people might use a 1 followed by many zeros, such as 1,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000, or use scientific notation, such as 10100
or 10googol
, where googol is another name for 10100
. However, these are just arbitrary representations, and there is no official or correct way to write bajillion.

Learned something new today!

Copy link
Member

Choose a reason for hiding this comment

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

@utpilla Don't worry about refactoring it on here. On #4958 I'm refactoring how these tests create their providers. Example: https://github.com/open-telemetry/opentelemetry-dotnet/pull/4958/files#diff-ae011cf18b74623dd9376d03c8629da33ab122c4d4b6a6c9d3f54325a13637eeR44

I'll have to merge these changes over there and I can encapsulate the config into the helper to achieve what @alanwest is asking for.

Copy link
Member

Choose a reason for hiding this comment

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

1,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000

Ok, I'll admit I exaggerated a bit... This prolly woulda broken GitHub.

@utpilla utpilla merged commit f2c2255 into open-telemetry:main Nov 16, 2023
70 checks passed
@utpilla
Copy link
Contributor Author

utpilla commented Dec 9, 2023

Related to #2360

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.

None yet

5 participants