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.

Sign up for GitHub

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

Minor refactor MetricReader and AggregationTemporality #2357

Merged
merged 2 commits into from
Sep 17, 2021
Merged

Minor refactor MetricReader and AggregationTemporality #2357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd06676
Minor refactor MetricReader and AggregationTemporality
reyang Sep 16, 2021
b780244
improve dispose
reyang Sep 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

namespace OpenTelemetry.Exporter
{
[AggregationTemporality(AggregationTemporality.Both, AggregationTemporality.Cumulative)]
[AggregationTemporality(AggregationTemporality.Cumulative | AggregationTemporality.Delta, AggregationTemporality.Cumulative)]
public class ConsoleMetricExporter : ConsoleExporter<Metric>
{
private Resource resource;
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricsExporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ namespace OpenTelemetry.Exporter
/// Exporter consuming <see cref="Metric"/> and exporting the data using
/// the OpenTelemetry protocol (OTLP).
/// </summary>
[AggregationTemporality(AggregationTemporality.Both, AggregationTemporality.Cumulative)]
[AggregationTemporality(AggregationTemporality.Cumulative | AggregationTemporality.Delta, AggregationTemporality.Cumulative)]
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, but I'm not following why the OTLP exporter is marked to prefer Cumulative. Seems to me the OTLP exporter should effectively not have a preference.

For a given service, is it currently not possible for some metrics to have a cumulative aggregation and others to have a delta aggregation?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, sorry, I think this is making sense to me now. I'm still trying to keep all these things in my head. I continue going back to a time when there was discussion about being able to define aggregation temporality at the level of an individual instrument or view. This is not the case.

Aggregation temporality is essentially defined at the level of a pipeline - so like MetricReader -> MetricExporter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me put some concrete examples to help us align the understanding:

Prometheus - only supports Cumulative (implied that Cumulative is also the preference):

  • The user doesn't have to specify temporality during the SDK configuration, everything will be exported as cumulative. In case the measurement being reported is Delta, there will be Delta->Cumulative conversion.
  • If the user specified Cumulative explicitly, it won't harm
  • If the user specified Delta explicitly, they will get some error telling them that the setup is wrong.

OTLP - supports both Cumulative and Delta (and Cumulative is the preference):

  • The user doesn't have to specify temporality during the SDK configuration, everything will be exported as Cumulative. In case the measurement being reported is Delta, there will be Delta->Cumulative conversion.
  • If the user specified Cumulative explicitly, it won't harm
  • If the user specified Delta explicitly, there are two cases:
    • If the value being reported is Delta (e.g. Counter), it'll be reported as Delta, no conversion required
    • If the value being reported is Cumulative (e.g. ObservableCounter), there will be Cumulative->Delta conversion.

Copy link
Member Author

Choose a reason for hiding this comment

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

@alanwest FYI open-telemetry/opentelemetry-specification#731

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for this context! Defaulting to cumulative had seemed somewhat arbitrary to me, but Prometheus + load-balanced collectors = a bad time if delta. Prometheus seems to make everything trickier 😄.

public class OtlpMetricsExporter : BaseOtlpExporter<Metric>
{
private readonly OtlpCollector.MetricsService.IMetricsServiceClient metricsClient;
Expand Down
10 changes: 0 additions & 10 deletions src/OpenTelemetry/Metrics/AggregationTemporality.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,5 @@ public enum AggregationTemporality : byte
/// Delta.
/// </summary>
Delta = 0b10,

reyang marked this conversation as resolved.
Show resolved Hide resolved
/// <summary>
/// Neither.
/// </summary>
Neither = 0b0,

/// <summary>
/// Both.
/// </summary>
Both = 0b11,
}
}
1 change: 1 addition & 0 deletions src/OpenTelemetry/Metrics/AggregationTemporalityAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

namespace OpenTelemetry.Metrics
{
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
public sealed class AggregationTemporalityAttribute : Attribute
{
private AggregationTemporality preferredAggregationTemporality;
Expand Down
16 changes: 10 additions & 6 deletions src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,8 @@ namespace OpenTelemetry.Metrics
public class BaseExportingMetricReader : MetricReader
{
protected readonly BaseExporter<Metric> exporter;
protected bool disposed;
reyang marked this conversation as resolved.
Show resolved Hide resolved

private readonly ExportModes supportedExportModes = ExportModes.Push | ExportModes.Pull;
private bool disposed;

public BaseExportingMetricReader(BaseExporter<Metric> exporter)
{
Expand Down Expand Up @@ -62,9 +61,12 @@ internal override void SetParentProvider(BaseProvider parentProvider)
/// <inheritdoc/>
protected override void Dispose(bool disposing)
{
base.Dispose(disposing);
if (this.disposed)
{
return;
}

if (disposing && !this.disposed)
if (disposing)
{
try
{
Expand All @@ -74,9 +76,11 @@ protected override void Dispose(bool disposing)
{
// TODO: Log
}

this.disposed = true;
}

this.disposed = true;

base.Dispose(disposing);
}
}
}
9 changes: 5 additions & 4 deletions src/OpenTelemetry/Metrics/MetricReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ namespace OpenTelemetry.Metrics
{
public abstract class MetricReader : IDisposable
{
private AggregationTemporality preferredAggregationTemporality = AggregationTemporality.Both;
private AggregationTemporality supportedAggregationTemporality = AggregationTemporality.Both;
private const AggregationTemporality CumulativeAndDelta = AggregationTemporality.Cumulative | AggregationTemporality.Delta;
reyang marked this conversation as resolved.
Show resolved Hide resolved
private AggregationTemporality preferredAggregationTemporality = CumulativeAndDelta;
private AggregationTemporality supportedAggregationTemporality = CumulativeAndDelta;

public BaseProvider ParentProvider { get; private set; }

Expand Down Expand Up @@ -74,12 +75,12 @@ protected virtual void Dispose(bool disposing)

private static void ValidateAggregationTemporality(AggregationTemporality preferred, AggregationTemporality supported)
{
if ((int)(preferred & AggregationTemporality.Both) == 0)
if ((int)(preferred & CumulativeAndDelta) == 0)
{
throw new ArgumentException($"PreferredAggregationTemporality has an invalid value {preferred}.", nameof(preferred));
}

if ((int)(supported & AggregationTemporality.Both) == 0)
if ((int)(supported & CumulativeAndDelta) == 0)
{
throw new ArgumentException($"SupportedAggregationTemporality has an invalid value {supported}.", nameof(supported));
}
Expand Down
10 changes: 9 additions & 1 deletion src/OpenTelemetry/Metrics/PeriodicExportingMetricReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public class PeriodicExportingMetricReader : BaseExportingMetricReader

private readonly Task exportTask;
private readonly CancellationTokenSource token;
private bool disposed;

public PeriodicExportingMetricReader(
BaseExporter<Metric> exporter,
Expand Down Expand Up @@ -62,7 +63,12 @@ public override void OnCollect(Batch<Metric> metrics)
/// <inheritdoc/>
protected override void Dispose(bool disposing)
{
if (disposing && !this.disposed)
if (this.disposed)
{
return;
}

if (disposing)
{
try
{
Expand All @@ -76,6 +82,8 @@ protected override void Dispose(bool disposing)
}
}

this.disposed = true;

base.Dispose(disposing);
}
}
Expand Down