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

Metric config modified to look similar to Tracing #2037

Merged
merged 7 commits into from
May 5, 2021

Conversation

cijothomas
Copy link
Member

Building on top of #2034 . Will be iterating on this :)

Changes:
Make configuration (MeterProviderBuilder) similar to Tracing configuration.

using var provider = Sdk.CreateMeterProviderBuilder()
                .AddSource("MeterName") // All instruments from this meter are enabled. Per instrument/more advanced can be added later.
                .SetObservationPeriod(observationInterval)
                .Build();

Added console example.

Next steps:
Add Resource to MeterProvider (similar to Tracing model)
Store the metrics into some inmemory exporter(?), and start a unit test framework.

@cijothomas cijothomas requested a review from a team May 5, 2021 19:43
@codecov
Copy link

codecov bot commented May 5, 2021

Codecov Report

Merging #2037 (50e0317) into metrics (c8d8b8c) will increase coverage by 0.00%.
The diff coverage is 89.47%.

Impacted file tree graph

@@           Coverage Diff            @@
##           metrics    #2037   +/-   ##
========================================
  Coverage    82.54%   82.54%           
========================================
  Files          205      208    +3     
  Lines         6461     6462    +1     
========================================
+ Hits          5333     5334    +1     
  Misses        1128     1128           
Impacted Files Coverage Δ
...elemetry/Metrics/MeterProviderBuilderExtensions.cs 66.66% <66.66%> (ø)
...c/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs 84.61% <80.00%> (-15.39%) ⬇️
src/OpenTelemetry.Api/Metrics/MeterProvider.cs 100.00% <100.00%> (ø)
.../OpenTelemetry.Api/Metrics/MeterProviderBuilder.cs 100.00% <100.00%> (ø)
src/OpenTelemetry/Metrics/MeterProviderSdk.cs 100.00% <100.00%> (ø)
src/OpenTelemetry/Sdk.cs 100.00% <100.00%> (ø)
...stem.Diagnostics.Metrics.Temp/ObservableCounter.cs 0.00% <0.00%> (-80.00%) ⬇️
src/System.Diagnostics.Metrics.Temp/Meter.cs 17.56% <0.00%> (ø)
... and 2 more

/// <param name="meterProviderBuilder"><see cref="MeterProviderBuilder"/>.</param>
/// <param name="periodMilliseconds">Perion in milliseconds.</param>
/// <returns><see cref="MeterProvider"/>.</returns>
public static MeterProviderBuilder SetObservationPeriod(this MeterProviderBuilder meterProviderBuilder, int periodMilliseconds)
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker for this PR.
I guess we will struggle with the ObservationPeriod since there are scenarios where different instruments being observed and reported at different frequency (e.g. during surgery, temperature is observed and reported every minute, blood pressure is observed and reported every 0.5 second, heart beat is observed every 0.1 second but reported every 15 seconds).

Copy link
Member Author

Choose a reason for hiding this comment

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

agree. Will keep this now to continue experimenting with this.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM as an initial step.

I've left some non-blocking comments that we want to revisit later.

@cijothomas cijothomas merged commit 4faf1d4 into metrics May 5, 2021
@cijothomas cijothomas deleted the cijothomas/metricprovider branch May 5, 2021 23:27
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