-
Notifications
You must be signed in to change notification settings - Fork 775
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
[DRAFT] fixing InMemoryExporter & Metrics bug. new class: ExportableMetricCopy. new ctor: InMemoryExporter(Func) #3198
Changes from all commits
6d1a88c
e8a496c
be840b5
26c0d4a
9fbc304
be30a48
9e09be3
357b3a3
c75dc01
c8cbd0a
1a28ae7
ed66ad5
a7641c4
6d40549
26d7e1f
9a0d99a
3a59585
f2b2062
8c06f94
5850db8
493520b
0027a4d
365b8cc
ec7ee18
f68b76e
42e4854
afb1d86
cd5f68d
fb94b9a
334779f
8dc80e6
054400d
534c674
3141ec3
942e952
e40acc5
38f0b6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,15 @@ | ||
OpenTelemetry.Exporter.InMemoryExporter<T>.InMemoryExporter(System.Func<OpenTelemetry.Batch<T>, OpenTelemetry.ExportResult> exportFunc) -> void | ||
OpenTelemetry.Metrics.ExportableMetricCopy | ||
OpenTelemetry.Metrics.ExportableMetricCopy.Description.get -> string | ||
OpenTelemetry.Metrics.ExportableMetricCopy.ExportableMetricCopy(OpenTelemetry.Metrics.Metric metric) -> void | ||
OpenTelemetry.Metrics.ExportableMetricCopy.MeterName.get -> string | ||
OpenTelemetry.Metrics.ExportableMetricCopy.MeterVersion.get -> string | ||
OpenTelemetry.Metrics.ExportableMetricCopy.MetricPoints.get -> System.Collections.Generic.IReadOnlyList<OpenTelemetry.Metrics.MetricPoint> | ||
OpenTelemetry.Metrics.ExportableMetricCopy.MetricType.get -> OpenTelemetry.Metrics.MetricType | ||
OpenTelemetry.Metrics.ExportableMetricCopy.Name.get -> string | ||
OpenTelemetry.Metrics.ExportableMetricCopy.Unit.get -> string | ||
OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions | ||
static OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions.AddInMemoryExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Collections.Generic.ICollection<OpenTelemetry.Metrics.Metric> exportedItems) -> OpenTelemetry.Metrics.MeterProviderBuilder | ||
static OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions.AddInMemoryExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Collections.Generic.ICollection<OpenTelemetry.Metrics.Metric> exportedItems, System.Action<OpenTelemetry.Metrics.MetricReaderOptions> configureMetricReader) -> OpenTelemetry.Metrics.MeterProviderBuilder | ||
static OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions.AddInMemoryExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Collections.Generic.ICollection<OpenTelemetry.Metrics.ExportableMetricCopy> exportedItems) -> OpenTelemetry.Metrics.MeterProviderBuilder | ||
static OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions.AddInMemoryExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Collections.Generic.ICollection<OpenTelemetry.Metrics.ExportableMetricCopy> exportedItems, System.Action<OpenTelemetry.Metrics.MetricReaderOptions> configureMetricReader) -> OpenTelemetry.Metrics.MeterProviderBuilder | ||
static OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions.AddInMemoryExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Func<OpenTelemetry.Batch<OpenTelemetry.Metrics.Metric>, OpenTelemetry.ExportResult> exportFunc) -> OpenTelemetry.Metrics.MeterProviderBuilder | ||
static OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions.AddInMemoryExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Func<OpenTelemetry.Batch<OpenTelemetry.Metrics.Metric>, OpenTelemetry.ExportResult> exportFunc, System.Action<OpenTelemetry.Metrics.MetricReaderOptions> configureMetricReader) -> OpenTelemetry.Metrics.MeterProviderBuilder |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,15 @@ | ||
OpenTelemetry.Exporter.InMemoryExporter<T>.InMemoryExporter(System.Func<OpenTelemetry.Batch<T>, OpenTelemetry.ExportResult> exportFunc) -> void | ||
OpenTelemetry.Metrics.ExportableMetricCopy | ||
OpenTelemetry.Metrics.ExportableMetricCopy.Description.get -> string | ||
OpenTelemetry.Metrics.ExportableMetricCopy.ExportableMetricCopy(OpenTelemetry.Metrics.Metric metric) -> void | ||
OpenTelemetry.Metrics.ExportableMetricCopy.MeterName.get -> string | ||
OpenTelemetry.Metrics.ExportableMetricCopy.MeterVersion.get -> string | ||
OpenTelemetry.Metrics.ExportableMetricCopy.MetricPoints.get -> System.Collections.Generic.IReadOnlyList<OpenTelemetry.Metrics.MetricPoint> | ||
OpenTelemetry.Metrics.ExportableMetricCopy.MetricType.get -> OpenTelemetry.Metrics.MetricType | ||
OpenTelemetry.Metrics.ExportableMetricCopy.Name.get -> string | ||
OpenTelemetry.Metrics.ExportableMetricCopy.Unit.get -> string | ||
OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions | ||
static OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions.AddInMemoryExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Collections.Generic.ICollection<OpenTelemetry.Metrics.Metric> exportedItems) -> OpenTelemetry.Metrics.MeterProviderBuilder | ||
static OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions.AddInMemoryExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Collections.Generic.ICollection<OpenTelemetry.Metrics.Metric> exportedItems, System.Action<OpenTelemetry.Metrics.MetricReaderOptions> configureMetricReader) -> OpenTelemetry.Metrics.MeterProviderBuilder | ||
static OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions.AddInMemoryExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Collections.Generic.ICollection<OpenTelemetry.Metrics.ExportableMetricCopy> exportedItems) -> OpenTelemetry.Metrics.MeterProviderBuilder | ||
static OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions.AddInMemoryExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Collections.Generic.ICollection<OpenTelemetry.Metrics.ExportableMetricCopy> exportedItems, System.Action<OpenTelemetry.Metrics.MetricReaderOptions> configureMetricReader) -> OpenTelemetry.Metrics.MeterProviderBuilder | ||
static OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions.AddInMemoryExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Func<OpenTelemetry.Batch<OpenTelemetry.Metrics.Metric>, OpenTelemetry.ExportResult> exportFunc) -> OpenTelemetry.Metrics.MeterProviderBuilder | ||
static OpenTelemetry.Metrics.InMemoryExporterMetricsExtensions.AddInMemoryExporter(this OpenTelemetry.Metrics.MeterProviderBuilder builder, System.Func<OpenTelemetry.Batch<OpenTelemetry.Metrics.Metric>, OpenTelemetry.ExportResult> exportFunc, System.Action<OpenTelemetry.Metrics.MetricReaderOptions> configureMetricReader) -> OpenTelemetry.Metrics.MeterProviderBuilder |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
// <copyright file="ExportableMetricCopy.cs" company="OpenTelemetry Authors"> | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// </copyright> | ||
|
||
using System.Collections.Generic; | ||
|
||
namespace OpenTelemetry.Metrics | ||
{ | ||
/// <summary> | ||
/// This class represents a selective copy of <see cref="Metric"/>. | ||
/// This contains the minimum fields and properties needed for most | ||
/// unit testing scenarios. | ||
/// </summary> | ||
public class ExportableMetricCopy | ||
{ | ||
private readonly MetricStreamIdentity instrumentIdentity; | ||
|
||
public ExportableMetricCopy(Metric metric) | ||
{ | ||
this.instrumentIdentity = metric.InstrumentIdentity; | ||
this.MetricType = metric.MetricType; | ||
|
||
List<MetricPoint> metricPoints = new(); | ||
foreach (ref readonly var metricPoint in metric.GetMetricPoints()) | ||
{ | ||
metricPoints.Add(metricPoint.Copy()); | ||
} | ||
|
||
this.MetricPoints = metricPoints; | ||
} | ||
|
||
public string Name => this.instrumentIdentity.InstrumentName; | ||
|
||
public string Description => this.instrumentIdentity.Description; | ||
|
||
public string Unit => this.instrumentIdentity.Unit; | ||
|
||
public string MeterName => this.instrumentIdentity.MeterName; | ||
|
||
public MetricType MetricType { get; } | ||
|
||
public string MeterVersion => this.instrumentIdentity.MeterVersion; | ||
|
||
public IReadOnlyList<MetricPoint> MetricPoints { get; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,7 +83,7 @@ public void Setup() | |
this.bounds[i] = i * MaxValue / this.bounds.Length; | ||
} | ||
|
||
var exportedItems = new List<Metric>(); | ||
var exportedItems = new List<ExportableMetricCopy>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it required to force every test to use the new MetricCopy unless they need to rely on it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, current approach was to force the use of the Copy. |
||
|
||
this.provider = Sdk.CreateMeterProviderBuilder() | ||
.AddMeter(this.meter.Name) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I follow the reason behind this throw...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is by design to force the use of the Copy.
The thought behind this was to prevent users from creating the
InMemoryExporter<Metric>
because theMetric
is known to be faulty. #3198 (comment)I've documented some scenarios where the
Metric
can and cannot be trusted: #2361 (comment)That being said, Prometheus and OTLP have some units tests where they need the actual
Metric
to provide to their own internals. (documented in the Summary above).I'm eager to hear your perspective on this!
I'm trying to find the happy medium to support all these use cases while protecting users.
If we decide to allow users to create
InMemoryExporter<Metric>
we also need to inform/educate users on the risks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to prevent users from using the one. A lot of tests are already using it, and working fine. (they understand the documented limitation of Metric that it may be updated after Export() call, but they are orchestrating things so as to not be affected)
This can be additional option, for InMemory users, who wants to test metrics, without being affected by the limitation that the memory could be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To unblock progress - lets split PR into sub Prs.