Metric aggregation public surface changes #323

vitalyf007 · 2016-10-06T18:48:10Z

This PR contains all proposed changes for AI SDK public surface to support metric aggregation.

dnduffy · 2016-10-06T19:16:47Z

src/Core/Managed/Shared/DataContracts/AggregatedMetricTelemetry.cs

+            this.Sum = Utils.SanitizeNanAndInfinity(this.Sum);
+            this.Min = Utils.SanitizeNanAndInfinity(this.Min);
+            this.Max = Utils.SanitizeNanAndInfinity(this.Max);
+            this.StandardDeviation = Utils.SanitizeNanAndInfinity(this.StandardDeviation);


Why not sanitize them on input (set) instead of as a separate step?

I just copied the way it was done in other types. Are you guys proposing to change it for all types (that would make sense)?

I don't have context on the history behind it but if there isn't a strong reason to do it like this I would propose changing it. Let's ask around in the morning.

I was looking more into this and I think I'd have to leave it the was it was coded originally. Sanitization is considered a part of the serialization, actually, and if we change it only for aggregated metric telemetry type, it will look odd and different to all other types. I'd prefer, if you like, to document (via a story) a future effort to clean that up across all telemetry types.

dnduffy · 2016-10-06T19:21:06Z

src/Core/Managed/Shared/Extensibility/MetricAggregatorManager.cs

+        /// <returns>Value aggregator for the metric specified.</returns>
+        public MetricAggregator CreateMetricAggregator(string metricName, IDictionary<string, string> dimensions = null)
+        {
+            throw new NotImplementedException();


Should I assume that the various "not implemented" methods will be implemented in the future?

Should I also assume that at some point the now marked as obsolete class MetricTelemetry will be deleted in favor of the new AggregatedMetricTelemetry?

This is a PR for public surface only (from one private branch to the other, not to develop yet). Naturally, "not implemented" methods will be implemented by the time we go to "develop".

As far as obsolete things - we can totally remove them in the next major version. For now they're marked obsolete and hidden from intellisense...

Okay, thank-you for confirming.

SergeyKanzhelev

I left couple of interesting comments

SergeyKanzhelev · 2016-10-14T21:34:51Z

src/Core/Managed/Shared/DataContracts/AggregatedMetricTelemetry.cs

+            this.Name = this.Name.SanitizeName();
+            this.Name = Utils.PopulateRequiredStringValue(this.Name, "name", typeof(AggregatedMetricTelemetry).FullName);
+            this.Properties.SanitizeProperties();
+            this.Count = this.Count < 0 ? 0 : this.Count;


should it be 1, not zero?

Some comment as above - why do you think so?

SET COUNT HERE TO "1"?

SergeyKanzhelev · 2016-10-14T21:35:15Z

src/Core/Managed/Shared/DataContracts/AggregatedMetricTelemetry.cs

+        /// </summary>
+        public int Count
+        {
+            get { return this.Metric.count ?? 0; }


should the default be 1, not zero?

I think it should be zero. Why do you think it should be 1?

My understanding is that Count represents the "base" for the value. It should not be zero.

+1. the Sanitize method will change it from 0 to 1 so the default value examined through the object will be different than the value sent on the wire..

Sanitize will set Count to 1 only in case when AggregatedMetricTelemetry was constructed w/o setting "Count" property (or explicitly setting it to <= 0). We consider this a mistake and I initially was setting it to "0" but that may present some problems in the pipeline...

The statement about different value on the wire and through property getter is not unique to this particular case.

I think that's why @SergeyKanzhelev and I are saying that Count should return this.Metric.count ?? 1. Otherwise Count returns incorrect value, 0, when it hasn't been set explicitly.

Fixed. Also added unit test.

SergeyKanzhelev · 2016-10-14T21:36:44Z

src/Core/Managed/Shared/Extensibility/Implementation/JsonSerializer.cs

@@ -236,9 +236,11 @@ private static void SerializeTelemetryItem(ITelemetry telemetryItem, JsonWriter
                ExceptionTelemetry exceptionTelemetry = telemetryItem as ExceptionTelemetry;
                SerializeExceptionTelemetry(exceptionTelemetry, jsonWriter);
            }
+#pragma warning disable CS0618


should you create a serializer for the AggregatedMetricTelemetry?

I did. I was omitted in this commit but will be there for the next one (will be available very shortly)

SergeyKanzhelev · 2016-10-14T21:40:06Z

src/Core/Managed/Shared/DataContracts/AggregatedMetricTelemetry.cs

+        /// <param name="min">Minimum value taken during aggregation interval.</param>
+        /// <param name="max">Maximum of values taken during aggregation interval.</param>
+        /// <param name="standardDeviation">Standard deviation of values taken during aggregation interval.</param>
+        public AggregatedMetricTelemetry(


Can you please expose the constructor without sum, min, max, etc. Mostly to report performance counter values, but also for the back compat

I expose parameter-less ctor that can be used along with property setting to populate whatever properties I like. I long considered adding others or making, say, min/max optional in some way on the ctor that takes all properties, but ruled that out at the end. I want user to explicitly state what they want to do and not omit anything. I think the schema I have right now is best, but we can discuss offline.

Why do we need this constructor? It's unreadable at the point of call.

new AggregatedMetricTelemetry("foo", 1, 2, 3, 4, 5);

You could make it readable by supplying parameter names, but you can achieve the same effect with property syntax.

new AggregatedMetricTelemetry("foo", count: 1, sum: 2, min: 3, max: 4, standardDeviation: 5); new AggregatedMetricTelemetry { Name = "Foo", Count = 1, Sum = 2, Min = 3, Max = 4, StandardDeviation = 5);

The ctor with parameters exists for the case when people aggregated entirely outside our Sdk and just want to send the results to AI. It states explicitly what is expected. If you use parameter-less ctor and property setters, it is not entirely clear what to set to get consistent results. This is somewhat against the purity of the design, but this is where we hit cold reality. We need to capture most common cases of providing metric data and make sure we do not create a large set of concepts. I thought having two concepts combined into one: a) metric that is just count (sum and count) and I do not care about the rest of the properties and b) metric where all properties are interested; would be Ok.

Therefore, people using parameter-less ctor may choose properties they want to set based on what they are going to look at. People using the other ctor know all things that need to be set if you want to go "comprehensive".

So we need a constructor that communicates intent but is unreadable when used. I guess I could argue this point both ways 😏. Let's keep it as is.

SergeyKanzhelev · 2016-10-14T21:42:12Z

src/Core/Managed/Shared/DataContracts/AggregatedMetricTelemetry.cs

+            this.Properties.SanitizeProperties();
+            this.Count = this.Count < 0 ? 0 : this.Count;
+            this.Sum = Utils.SanitizeNanAndInfinity(this.Sum);
+            this.Min = Utils.SanitizeNanAndInfinity(this.Min);


this call will initialize nullable field. Which is bad as we will need to pass the value over the wire even if we do not want to. Typical example is performance counter value. I do not want to spent bytes over the wire to send the min and max which are equal to the current value.

Also add a unit test that will ensure that not setting min and max will result to not serializing of those

I still think those fields should be optional and we do not need to send them if they are not available. Let's discuss

vitalyf007 · 2016-10-19T23:41:34Z

Please note an update on this PR. This is a "nearly" final version. I need to think about a couple of things here and I have tracing not yet done. Otherwise, unless you point out holes and/or problems I propose we consider this as a "we're done" stage and try use it in internal efforts such as Sergey's exception stats feature.

…etry

…end. Added tracing

…same aggregator instance for the same metric name and dimensions and does not create new aggregator instance on each call. Also changed method name.

SergeyKanzhelev

I didn't find where you limit the number of dimensions as we discussed on the previous sync up, But it may be a separate PR

SergeyKanzhelev · 2016-11-03T19:25:32Z

src/Core/Managed/Shared/Extensibility/MetricAggregatorManager.cs

+        /// <summary>
+        /// Name of the property added to aggregation results to indicate duration of the aggregation interval.
+        /// </summary>
+        private static string intervalDurationPropertyName = "IntervalDurationMs";


Why do we need it? I thought we agreed that we assume 1 minute as a default interval.

As discussed, will keep to address the shortened intervals at the start and shutdown times for the application.

SergeyKanzhelev · 2016-11-03T19:33:50Z

src/Core/Managed/Shared/Extensibility/Implementation/Tracing/CoreEventSource.cs

+        [Event(
+            27,
+            Message = "Failed to invoke metric processor. Exception: {0}.",
+            Level = EventLevel.Error)]


Level "Error" means that this trace will be sent to the portal as SDK "self-diagnostics" event. To make it more understandable for the user I'd recommend to mark it "User Actionable", trace the processor name and write instruction that "if issue persists - disable this processor"

ERROR IS CUSTOMER FACING. Get type of the processor. User-actionable. Suggest turning off processor if persists.

SergeyKanzhelev · 2016-11-03T19:35:52Z

src/Core/Managed/Shared/Extensibility/MetricAggregatorManager.cs

+
+            this.cancellationSource = new CancellationTokenSource();
+
+            this.snapshotTask = new Task(this.SnapshotRunner, TaskCreationOptions.LongRunning);


It makes the third thread SDK runs in background. QP configuration, Performance Counters collection and this aggregator. Can you think of any options how to combine this thread with Performance Counters collection thread?

It is lower priority comment, we can convert it to "enhancement" issue

Will need to address later if these keep breading...

SergeyKanzhelev · 2016-11-03T19:42:19Z

src/Core/Managed/Shared/DataContracts/AggregatedMetricTelemetry.cs

+            this.Properties.SanitizeProperties();
+            this.Count = this.Count < 0 ? 0 : this.Count;
+            this.Sum = Utils.SanitizeNanAndInfinity(this.Sum);
+            this.Min = Utils.SanitizeNanAndInfinity(this.Min);


I still think those fields should be optional and we do not need to send them if they are not available. Let's discuss

SergeyKanzhelev · 2016-11-03T19:48:42Z

src/Core/Managed/Shared/Extensibility/TelemetryConfiguration.cs

+        /// Gets the list of <see cref="IMetricProcessor"/> objects used for custom metric data processing
+        /// before client-side metric aggregation process.
+        /// </summary>
+        public IList<IMetricProcessor> MetricProcessors


Name processor makes it inconsistent with the TelemetryProcessors which combines the chain rather than sequential processing. Do you have any other names in mind?

I was changed to "processor" from "metric stream listener"to make it more consistent :) I'll leave it as is for now.

I agree with Sergey. Suggestion:
type name: IMetricValueTracker
property name: IList RawMetricValueTrackers

SergeyKanzhelev · 2016-11-03T19:52:33Z

src/Core/Managed/Shared/Extensibility/MetricAggregator.cs

+        /// <summary>
+        /// Aggregator manager for the aggregator.
+        /// </summary>
+        private MetricAggregatorManager manager;


make readonly

SergeyKanzhelev · 2016-11-03T19:52:52Z

src/Core/Managed/Shared/Extensibility/MetricAggregator.cs

+        /// <summary>
+        /// Metric aggregator id to look for in the aggregator dictionary.
+        /// </summary>
+        private string aggregatorId;


SergeyKanzhelev · 2016-11-03T19:52:57Z

src/Core/Managed/Shared/Extensibility/MetricAggregator.cs

+        /// <summary>
+        /// Aggregator hash code.
+        /// </summary>
+        private int hashCode;


SergeyKanzhelev · 2016-11-04T16:33:27Z

src/Core/Managed/Shared/DataContracts/AggregatedMetricTelemetry.cs

+
+            // note: we set count to 1 if it isn't a postitive integer
+            // thinking that if it is zero (negative case is clearly broken)
+            // that most likely menas somebody set the sum but forgot to set count


mistype "menas" instead of "means"

olegsych · 2016-11-06T17:04:46Z

src/Core/Managed/Shared/DataContracts/AggregatedMetricTelemetry.cs

+        /// <summary>
+        /// Gets or sets the min value of this metric.
+        /// </summary>
+        public double Min


Does the non-nullable double type indicate that Count, Min, Max and StandardDeviation properties are required to have value? With the current implementation of Sanitize it is possible to supply Sum and Count values only, and have the Min, Max and StandardDeviation values "default" to 0. Is that indentional?

That was intentional.I do not see a value in making these nullable. Nullable things are more difficult to work with and AI UX does not treat these any different than 0s. Effectively, we again getting into case where we're trying to represent multiple concepts via one class and therefore > 50% of it is nullable...

Good point. Thanks for the explanation.

olegsych · 2016-11-06T17:19:08Z

src/Core/Managed/Shared/Extensibility/MetricAggregatorManager.cs

+        /// <summary>
+        /// Metric aggregator snapshotting task.
+        /// </summary>
+        private Task snapshotTask;


Is it possible to reuse the TaskTimer instead?

I wasn't aware of TaskTimer existence. The answer is "almost". TaskTimer fires at regular intervals while perf counter reporting tasks changes interval after each invocation if needed. We can change the TaskTimer, of course, of create a separate class for this one usage...

I think TaskTimer fires once, once the TimeSpan specified by its Delay property elapses. You can adjust the Delay property from the elapsed function and start again with the new delay. With the benefit of perfect hindsight, this is not the most elegant design, but I think it should get the job done. I do think that timer logic is tricky enough to invest in a single robust implementation that's easy to use for aggregation as well.

I looked at TaskTimer. I understand what it is doing and it would [almost] work for me if I were to have functionality (in it) to do something if wait is terminated (I need to try to do Flush() to attempt not to lose any telemetry I can salvage). I considered the problem and I think it is too large to be coupled with the change I'm doing.

I suggest separating the area that covers two questions: A) How many timer implementations does one library need? and B) How many utility threads does one library need to run? and tackling it separately. We also have a perfect guy to propose the best solution - Greg. I chatted with him and once he is done with his current thing (should be just a few days) he'll look into this.

I'll take that back. Don't know what I was thinking. I can do it a lot simpler. Will use TaskTimer

olegsych · 2016-11-06T17:25:13Z

src/Core/Managed/Shared/Extensibility/MetricAggregatorManager.cs

+        /// <summary>
+        /// Cancellation token source to allow cancellation of the snapshotting task.
+        /// </summary>
+        private CancellationTokenSource cancellationSource;


Can any of these fields be readonly?

Yes, token and the task itself. Added readonly attributes

olegsych · 2016-11-06T17:58:05Z

src/Core/Managed/Shared/Extensibility/MetricAggregator.cs

+        {
+            // create a local reference to metric processor collection
+            // if collection changes after that - it will be copied not affecting local reference
+            IList<IMetricProcessor> metricProcessors = this.manager.Client.TelemetryConfiguration.MetricProcessors;


Ouch. This is a pretty bad violation of the law of Demeter. From customer viewpoint, this means that in order to test the use of MetricAggregator in my code, I'll need to understand a chain of objects 4 levels deep and create it for every test. In practice, I think few people will go through the effort successfully and, like me, will decide to wrap the MetricAggregator and the MetricAggregatorManager in application abstractions as "untestable external dependencies" instead.

Aside from the testability aspect, this is a pretty strong design smell of MetricAggregator performing responsibilities that belong somewhere else, perhaps in the SimpleMetricStatisticAggregator or in a class that doesn't exist yet, like a MessageProcessorForwarder: IMetricProcessor.

This is the same pattern as with regular .Net events. May not be necessary. Lets chat about it...

I made a fix for this. Technically I'm shifting responsibilities by encapsulating a bit more. Do you see this as acceptable fix (or at least a step in the right direction) or more of a hack?

olegsych · 2016-11-06T18:05:51Z

src/Core/Managed/Shared/Extensibility/MetricAggregator.cs

+    /// <summary>
+    /// Represents aggregator for a single time series of a given metric.
+    /// </summary>
+    public class MetricAggregator : IEquatable<MetricAggregator>


Are we implementing IEquatable in MetricAggregator only for the sake of the MetricAggregatorManager.GetStatisticsAggregator? It's a lot more (untested 😱) code in the MetricAggregator and harder to understand in the MetricAggregatorManager. Why not simply use MetricAggregator.aggregatorId directly?

I did use aggregator id directly initially. Then I decided and can cache the hash code speeding up lookups. That idea led to current design...

Good point. I'll settle for unit tests then 😄

Tests added.

olegsych · 2016-11-06T18:24:51Z

Test/CoreSDK.Test/Shared/Extensibility/MetricAggregatorManagerTest.cs

+    public class MetricAggregatorManagerTest
+    {
+        [TestMethod]
+        public void MetricAggregatorMayBeCreatedForMetricHavingNoDimensions()


I felt these integration tests were hard to understand for someone who is not already familiar with all 3 classes implementing metric aggregation. Arrange, Act, Assert comments would help. If it's possible at this point, factoring the classes to allow unit testing them separately would help more.

I do not entirely understand. We'll need to chat offline.

Good idea. For readability, I'd be happy if the tests looked like this.

public void TestName() { // Arrange ... // Act ... // Assert ... }

Added comments where I thought it was not obvious.

Thanks, @vitalyf007! I feel these tests are much easier to read with the arrange/act/assert comments.

olegsych · 2016-11-06T18:26:54Z

Test/CoreSDK.Test/Shared/Extensibility/MetricAggregatorTest.cs

+    using Assert = Xunit.Assert;
+
+    [TestClass]
+    public class MetricAggregatorTest


Are we missing tests for the IEquatable implementation?

There is a set of tests in MetricAggregatorManager that tests these indirectly, however, I can (and likely should) add MA equality tests.

Tests added and existing ones are refactored. Overall looks a lot cleaner.

olegsych · 2016-11-06T18:38:20Z

src/Core/Managed/Shared/Extensibility/Implementation/SimpleMetricStatisticsAggregator.cs

+    /// <summary>
+    /// Represents aggregator for a single time series of a given metric.
+    /// </summary>
+    internal class SimpleMetricStatisticsAggregator


As I was reading this code, I felt that two aggregator classes, MetricAggregator and SimpleMetricStatisticAggregator are confusing. What if they were called Metric and MetricAggregator respectively? As a potential benefit, this naming would allow to simplify the IMetricProcessor definition to Track(Metric, double value), which would also be more resilient to future changes in the Metric class.

I need to think about Metric/MetricAggregator naming...

You're almost right. There are two aggregators, so to speak. MetricAggregator is really a "handle" or a proxy to te actual aggregator. You can hold on to it aslong as you like and operating on it will be proxied to actual aggregator which is SimpleMetricStatisticsAggregator class.
Note that metric data may be aggregated in a number of ways. Extracting/computing simple stats is just a way. You can also do t-digest and some other fun stuff too. Lets chat more about it.

Ok, so how about calling them Metric and SimpleMetricStatisticAggregator then?

I'd say we have to settle on the names of three things. Currently I have MetricAggregator, MetricAggregatorManager (this one with CreateMetricAggregator method) and SimpleMetricStatisticsAggregator.
I guess you're suggesting the following naming schema: Metric, MetricManager(with CreateMetricmethod) and SimpleMerticStatisticsAggregator(no change on the last one)?

olegsych

@vitalyf007 Thanks for the pull request. It's great to see the metric aggregation making its way to the core SDK. I have a couple of concerns about implementation of the new AggregatedMetricTelemetry class and an alternative naming for the MetricAggregator I would like you to consider. The remaining comments are small implementation concerns and suggestions. Could you take a look at my comments?

cc: @SergeyKanzhelev

SergeyKanzhelev · 2016-11-07T22:50:07Z

src/Core/Managed/Shared/Extensibility/MetricAggregatorManager.cs

+
+                    if (aggergatedMetricTelemetry != null)
+                    {
+                        this.Client.Track(aggergatedMetricTelemetry);


I just thought about it - I'd suggest to set an sdkVersion here different from just dotnet: XX.XX.XX. Something like agg-net: XX.XXX.XX so you'll now how many metrics were collected using this aggregation mechanism.

Are you suggesting setting a special tag for this specific way of aggregating such that we can tell how many people use it? I do not think version of the Sdk is the right way though...

…m 0 to 1. Plus unit test

olegsych · 2016-11-12T18:56:01Z

src/Core/Managed/Shared/Extensibility/MetricAggregatorManager.cs

+        /// <returns>Metric telemetry object resulting from aggregation.</returns>
+        private static AggregatedMetricTelemetry CreateAggergatedMetricTelemetry(MetricAggregator aggregator, SimpleMetricStatisticsAggregator aggregatorStats)
+        {
+            if ((aggregator == null) || (aggregatorStats == null) || (aggregatorStats.Count <= 0))


Can the aggregator or aggregatorStats be ever null here when they come from this.aggregatorDictionary protected by the logic in GetStatisticsAggregator?

They cannot be, correct. If favor of failing fast, I'll remove the check.

olegsych · 2016-11-12T18:57:09Z

src/Core/Managed/Shared/Extensibility/MetricAggregatorManager.cs

+                {
+                    AggregatedMetricTelemetry aggergatedMetricTelemetry = CreateAggergatedMetricTelemetry(aggregatorWithStats.Key, aggregatorWithStats.Value);
+
+                    aggergatedMetricTelemetry.Properties.Add(intervalDurationPropertyName, ((long)aggregationIntervalDuation.TotalMilliseconds).ToString(CultureInfo.InvariantCulture));


The aggregatedMetricTelemetry can be null here if the aggregator's Count is 0. We should add the intervalDurationPropertyName inside the if statement below. Ideally, I think the CreateAggregateMetricTelemetry should always return a valid object and Snapshot should check for Count == 0 instead of null to make it clear why the AggregatedMetricTelemetry is not created and not tracked.

:( Good catch. Fixed

olegsych · 2016-11-12T19:04:29Z

src/Core/Managed/Shared/Extensibility/MetricAggregatorManager.cs

+                    }
+                    catch (Exception ex)
+                    {
+                        CoreEventSource.Log.FailedToSnapshotMetricAggregators(ex.ToString());


The TaskTimer already logs unhandled exceptions. Do we really need the additional logging of FailedToSnapshotMetricAggregators?

I'd rather have a specific message logged here in addition to more generic "timed task failed" message logged by timed task.

olegsych · 2016-11-12T19:07:12Z

src/Core/Managed/Shared/Extensibility/MetricAggregatorManager.cs

+                throw new ObjectDisposedException(nameof(MetricAggregatorManager));
+            }
+
+            if (aggregator == null)


The GetStatisticsAggregator is only called by MetricAggregator.Track. Can aggregator be ever null?

No. Will remove the check.

olegsych

@vitalyf007 Thanks for addressing my previous comments. I've completed a second review pass, unfortunately only for product code this time as I need to cut this short today. I think we have to address the potential NullReferenceException in the Snapshot method of the MetricAggregatorManager. Other than that, I left a few comments in places where I found what looks like defensive code and would prefer it to fail fast instead. Plus some questions about tests and style (it's my OCD, I know, I just can't help it!) I feel strongly only about the NullReferenceException problem. Please ping me if/when you want me to do another review pass.

Drop "aggregator" from names

macrogreg · 2016-11-15T23:04:01Z

src/Core/Managed/Shared/Extensibility/Implementation/SimpleMetricStatisticsAggregator.cs

+    /// Represents mechanism to calculate basic statistical parameters of a series of numeric values.
+    /// </summary>
+    internal class SimpleMetricStatisticsAggregator
+    {


I have note yet read the whole CR so I may learn this shortly (sorry if that's the case), but what do you use these getters for and how important is it that they give consistent values? You may want to put them under lock too.

So having read the other bits, the only way this can be a problem is when Track was called right before a Snapshot is made and then lost the race. We would expose the spinlock internally and lock the aggregator while making the snapshot. Because the race is very rare, the lock will be contended very rarely and so should not result in any significant perf decrease.

Yes, given the read pattern (once right after snapshot) I do not think this is a problem. I do not want to complicate getters with honoring the lock especially considering the fact that they'll be called sequentially.

macrogreg · 2016-11-15T23:09:27Z

src/Core/Managed/Shared/Extensibility/Metric.cs

+                return false;
+            }
+
+            return string.Compare(this.aggregatorId, other.aggregatorId, StringComparison.Ordinal) == 0;


Minor: Implement Equals using Equals (of the aggregatorId)

macrogreg · 2016-11-15T23:14:43Z

src/Core/Managed/Shared/Extensibility/Metric.cs

+        private static string GetAggregatorId(string name, IDictionary<string, string> dimensions = null)
+        {
+            StringBuilder aggregatorIdBuilder = new StringBuilder(name ?? string.Empty);
+


Consider a different string constant than "" here. This might help debugging. Can name actually be empty? If yes, we will confuse it with a null name. Maybe "MetricNameIsNull" or somthing?

Will use "n/a" as that is the way Sdk represents other names (such as metric name, event name) during serialization if the name is null.

macrogreg · 2016-11-15T23:15:40Z

src/Core/Managed/Shared/Extensibility/Metric.cs

+
+                foreach (KeyValuePair<string, string> pair in sortedDimensions)
+                {
+                    aggregatorIdBuilder.AppendFormat(CultureInfo.InvariantCulture, "\n{0}\t{1}", pair.Key ?? string.Empty, pair.Value ?? string.Empty);


You are using /n and /t as separators. Are we sure that they cannot be part of the key?

They can appear in the name but I do not expect to ever see two real cases of that. Metric names, dimension names and values appear in the UX and generally speaking expected to be symbols and spaces only...

macrogreg · 2016-11-15T23:16:57Z

src/Core/Managed/Shared/Extensibility/Metric.cs

+
+            if (metricProcessors != null)
+            {
+                foreach (IMetricProcessor processor in metricProcessors)


This is hot path, right?
If so, maybe a For Loop is more performant?

Yes, good catch. Thanks.

macrogreg · 2016-11-15T23:36:55Z