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

Refactor aggregator and exporters #2295

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
125 changes: 74 additions & 51 deletions src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

namespace OpenTelemetry.Exporter
{
public class ConsoleMetricExporter : ConsoleExporter<MetricItem>
public class ConsoleMetricExporter : ConsoleExporter<Metric>
{
private Resource resource;

Expand All @@ -32,7 +32,7 @@ public ConsoleMetricExporter(ConsoleExporterOptions options)
{
}

public override ExportResult Export(in Batch<MetricItem> batch)
public override ExportResult Export(in Batch<Metric> batch)
Copy link
Member

@reyang reyang Sep 1, 2021

Choose a reason for hiding this comment

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

I think 1) metrics exporter will always run in a separate thread (unlike trace/log exporter which might run in the hot path - e.g. SimpleExportingProcessor) 2) metrics exporter will only be triggered at a much lower frequency (unlike trace/log exporter which might get triggered for every single event), so it might be actually easier if we define this as IEnumerable<Metric>.

I understand that there might be a desire to have ConsoleExporter<T> which covers logs/metrics/traces (for sake of consistency).

@CodeBlanch do you have strong opinion on this? (look at the change on the src/OpenTelemetry/Batch.cs, do you think the extra if-condition could slow down traces/logs?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes agree to make this simpler with just IEnumerable, so we won't affect the other signals's hot path.

Copy link
Member

@CodeBlanch CodeBlanch Sep 2, 2021

Choose a reason for hiding this comment

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

Could we do IList<T> instead of IEnumerable<T>? With that we could at least do a for (int i = 0; i < list.Count; i++) { var item = list[i]; } type of loop that doesn't allocate an enumerator. Or we could even pass the array directly. Array doesn't have a struct enumerator but AFAIK the JIT is smart enough to re-write a foreach loop on an array to be allocation-free.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if we will be able to tell the count of metrics or not, if we could, IList definitely works better.

I guess array won't work since it assumes certain memory layout. For example, if we pre-allocate memory and keep reusing them, we would need to compact the pre-allocated aggregation buffer before exporting (and that seems to be expensive).

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

We could also pass a ReadOnlySpan over the array?

{
if (this.resource == null)
{
Expand All @@ -49,101 +49,124 @@ public override ExportResult Export(in Batch<MetricItem> batch)
}
}

foreach (var metricItem in batch)
foreach (var metric in batch)
{
foreach (var metric in metricItem.Metrics)
var msg = new StringBuilder($"\nExport ");
msg.Append(metric.Name);
if (!string.IsNullOrEmpty(metric.Description))
{
var tags = metric.Attributes.ToArray().Select(k => $"{k.Key}={k.Value?.ToString()}");
msg.Append(' ');
msg.Append(metric.Description);
}

if (!string.IsNullOrEmpty(metric.Unit))
{
msg.Append($", Unit: {metric.Unit}");
}

if (!string.IsNullOrEmpty(metric.Meter.Name))
{
msg.Append($", Meter: {metric.Meter.Name}");

if (!string.IsNullOrEmpty(metric.Meter.Version))
{
msg.Append($"/{metric.Meter.Version}");
}
}

Console.WriteLine(msg.ToString());

foreach (var metricPoint in metric.GetMetricPoints())
{
string valueDisplay = string.Empty;
StringBuilder tagsBuilder = new StringBuilder();
for (int i = 0; i < metricPoint.Keys.Length; i++)
{
tagsBuilder.Append(metricPoint.Keys[i]);
tagsBuilder.Append(":");
tagsBuilder.Append(metricPoint.Values[i]);
}

var tags = tagsBuilder.ToString();

// Switch would be faster than the if.else ladder
// of try and cast.
switch (metric.MetricType)
{
case MetricType.LongSum:
{
valueDisplay = (metric as ISumMetricLong).LongSum.ToString(CultureInfo.InvariantCulture);
valueDisplay = metricPoint.LongValue.ToString(CultureInfo.InvariantCulture);
break;
}

case MetricType.DoubleSum:
{
valueDisplay = (metric as ISumMetricDouble).DoubleSum.ToString(CultureInfo.InvariantCulture);
valueDisplay = metricPoint.DoubleValue.ToString(CultureInfo.InvariantCulture);
break;
}

case MetricType.LongGauge:
{
valueDisplay = (metric as IGaugeMetric).LastValue.Value.ToString();
valueDisplay = metricPoint.LongValue.ToString(CultureInfo.InvariantCulture);
break;
}

case MetricType.DoubleGauge:
{
valueDisplay = (metric as IGaugeMetric).LastValue.Value.ToString();
valueDisplay = metricPoint.DoubleValue.ToString(CultureInfo.InvariantCulture);
break;
}

case MetricType.Histogram:
{
var histogramMetric = metric as IHistogramMetric;
var bucketsBuilder = new StringBuilder();
bucketsBuilder.Append($"Sum: {histogramMetric.PopulationSum} Count: {histogramMetric.PopulationCount} \n");
foreach (var bucket in histogramMetric.Buckets)
bucketsBuilder.Append($"Sum: {metricPoint.DoubleValue} Count: {metricPoint.LongValue} \n");
for (int i = 0; i < metricPoint.ExplicitBounds.Length + 1; i++)
{
bucketsBuilder.Append('(');
bucketsBuilder.Append(double.IsInfinity(bucket.LowBoundary) ? "-Infinity" : $"{bucket.LowBoundary}");
bucketsBuilder.Append(", ");
bucketsBuilder.Append(double.IsInfinity(bucket.HighBoundary) ? "+Infinity" : $"{bucket.HighBoundary}");
bucketsBuilder.Append(double.IsInfinity(bucket.HighBoundary) ? ')' : ']');
bucketsBuilder.Append($": {bucket.Count}");
if (i == 0)
{
bucketsBuilder.Append("(-Infinity,");
bucketsBuilder.Append(metricPoint.ExplicitBounds[i]);
bucketsBuilder.Append("]");
bucketsBuilder.Append(":");
bucketsBuilder.Append(metricPoint.BucketCounts[i]);
}
else if (i == metricPoint.ExplicitBounds.Length)
{
bucketsBuilder.Append("(");
bucketsBuilder.Append(metricPoint.ExplicitBounds[i - 1]);
bucketsBuilder.Append(",");
bucketsBuilder.Append("+Infinity]");
bucketsBuilder.Append(":");
bucketsBuilder.Append(metricPoint.BucketCounts[i]);
}
else
{
bucketsBuilder.Append("(");
bucketsBuilder.Append(metricPoint.ExplicitBounds[i - 1]);
bucketsBuilder.Append(",");
bucketsBuilder.Append(metricPoint.ExplicitBounds[i]);
bucketsBuilder.Append("]");
bucketsBuilder.Append(":");
bucketsBuilder.Append(metricPoint.BucketCounts[i]);
}

bucketsBuilder.AppendLine();
}

valueDisplay = bucketsBuilder.ToString();
break;
}

case MetricType.Summary:
{
var summaryMetric = metric as ISummaryMetric;
valueDisplay = string.Format("Sum: {0} Count: {1}", summaryMetric.PopulationSum, summaryMetric.PopulationCount);
break;
}
}

var msg = new StringBuilder($"Export (");
msg.Append(metric.StartTimeExclusive.ToString("yyyy-MM-ddTHH:mm:ss.fffffffZ", CultureInfo.InvariantCulture));
msg = new StringBuilder();
msg.Append(metricPoint.StartTime.ToString("yyyy-MM-ddTHH:mm:ss.fffffffZ", CultureInfo.InvariantCulture));
msg.Append(", ");
msg.Append(metric.EndTimeInclusive.ToString("yyyy-MM-ddTHH:mm:ss.fffffffZ", CultureInfo.InvariantCulture));
msg.Append(metricPoint.EndTime.ToString("yyyy-MM-ddTHH:mm:ss.fffffffZ", CultureInfo.InvariantCulture));
msg.Append("] ");
msg.Append(metric.Name);
msg.Append(' ');
msg.Append(string.Join(";", tags));
msg.Append(' ');
msg.Append(metric.MetricType);

if (!string.IsNullOrEmpty(metric.Description))
{
msg.Append($", Description: {metric.Description}");
}

if (!string.IsNullOrEmpty(metric.Unit))
{
msg.Append($", Unit: {metric.Unit}");
}

if (!string.IsNullOrEmpty(metric.Meter.Name))
{
msg.Append($", Meter: {metric.Meter.Name}");

if (!string.IsNullOrEmpty(metric.Meter.Version))
{
msg.Append($"/{metric.Meter.Version}");
}
}

msg.AppendLine();
msg.Append($"Value: {valueDisplay}");
Console.WriteLine(msg);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public static class InMemoryExporterMetricHelperExtensions
/// <param name="configure">Exporter configuration options.</param>
/// <returns>The instance of <see cref="MeterProviderBuilder"/> to chain the calls.</returns>
[System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "The objects should not be disposed.")]
public static MeterProviderBuilder AddInMemoryExporter(this MeterProviderBuilder builder, ICollection<MetricItem> exportedItems, Action<InMemoryExporterOptions> configure = null)
public static MeterProviderBuilder AddInMemoryExporter(this MeterProviderBuilder builder, ICollection<Metric> exportedItems, Action<InMemoryExporterOptions> configure = null)
{
if (builder == null)
{
Expand All @@ -44,7 +44,7 @@ public static MeterProviderBuilder AddInMemoryExporter(this MeterProviderBuilder

var options = new InMemoryExporterOptions();
configure?.Invoke(options);
return builder.AddMetricProcessor(new PushMetricProcessor(new InMemoryExporter<MetricItem>(exportedItems), options.MetricExportIntervalMilliseconds, options.IsDelta));
return builder.AddMetricProcessor(new PushMetricProcessor(new InMemoryExporter<Metric>(exportedItems), options.MetricExportIntervalMilliseconds, options.IsDelta));
}
}
}
Loading