Feature/PreAggMetrics.Master #735
Conversation
…trics project and first draft of a MultidimensionalCube implementation.
…rgely complete. Not tested.
…o prevent false warnings.
…ditional tests and fixes.
|
@SergeyKanzhelev I have pushed a change that re-enables PublicApiAnalyzer and lists the APIs in the right files. |
| @@ -140,6 +140,7 @@ public void TrackMetricSendsSpecifiedAggregatedMetricTelemetry() | |||
| var sentTelemetry = new List<ITelemetry>(); | |||
| var client = this.InitializeTelemetryClient(sentTelemetry); | |||
|
|
|||
| #pragma warning disable CS0618 // Type or member is obsolete | |||
There was a problem hiding this comment.
Discouraging doesn't mean you need to deprecate it. You can make it non editor browseable. I was OK obsoleting it when we discussed introducing new TrackAggregaterMetric API. Since we still have a VALID scenario when one will want to use API - we should not mark it as obsolete.
| @@ -0,0 +1,37 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
Not OK. Either code follows all practices and builds in a single build or should be moved out of repository
| @@ -0,0 +1,61 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
Please see my reply there
| /// <param name="value">Value to be checked.</param> | ||
| /// <param name="name">Name of the parameter being checked.</param> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static void ValidateNotNull(object value, string name) |
There was a problem hiding this comment.
value ?? throw new ArgumentNullException(paramName: nameof(value); Or any of examples in this repo like https://github.com/Microsoft/ApplicationInsights-dotnet/blob/7321d2bf7b9c4733fb43b6c186e381863931eb1a/src/ServerTelemetryChannel/Implementation/PlatformFile.cs#L12-L15
|
|
||
| namespace Microsoft.ApplicationInsights.Metrics.Extensibility | ||
| { | ||
| /// <summary /> |
There was a problem hiding this comment.
then move the project out please
| } | ||
| } | ||
|
|
||
| #pragma warning disable SA1401 // Fields must be private |
There was a problem hiding this comment.
So why not properties? Compiler will optimize everything out
| } | ||
|
|
||
| /// <summary> | ||
| /// Groups constants used by metric aggregates produced by aggregators that are configured by metric configurations represented through |
|
Hi @Dmitry-Matveev & @SergeyKanzhelev , I have critically evaluated the new API surface. However, if we hold off on releasing this feature in the first beta, we can significantly reduce the size of the public API surface. We can manage the size of the public change by doing this. However, I would like to re-enable the respective APIs in a future beta. Are you OK with this plan? |
…cExposureCandidate.
|
I have pushed a change that reduces the public surface significantly, as discussed in my previous comments. Please note that there is still a significant number of lines in the PublicAPI.Unshipped.txt files, but a lot of them are overloads of the same methods. I gave the remaining public APIs a quick scan (it was quick, the below list may have some errors, but the order-of-magnitude point is valid). I removed overloads and counted the actual new APIs. Results:
All in all, not that much :) Heads-up: @SergeyKanzhelev @Dmitry-Matveev |
…ust match accessors".
…be copied and pasted".
…ation instead of using ConditionalWeakTable to map.
|
Thank you, @Dmitry-Matveev , for the review.
@SergeyKanzhelev : You have asked some questions to which I all replied. Here is a link to that conversation for your convenience: Do you have any follow-up questions? Thanks! |
| Microsoft.ApplicationInsights.Metric |
There was a problem hiding this comment.
historically we keep namespace Microsoft.ApplicationInsights clean. So developer has only option in intelliSense after typing Microsoft.ApplicaitonInsights to create a TelemetryClient. I remember we looked at implementation and only thing in root namespace was change in TelemetryClient. Is my recollection incorrect?
There was a problem hiding this comment.
Of course I may not have the right historical perspective.
However, this is not quite correct:
I remember we looked at implementation and only thing in root namespace was change in TelemetryClient.
The public API service is identical to what we reviewed at the end of 2017 (barring the types that we made internal this week as a follow up to @Dmitry-Matveev 's feedback). At the time, I made a set of examples available, which are now also in the Core repo:
In that set of examples, you can find use case that get progressively more advanced. The goal was to enable the use cases from example 1 to be discovered via Intellisense without the need for additional namespace imports. In order to achieve that, the base namespace includes all types that may appear as parameters to the metric related TelemetryClient methods.
Please let me know if this answers your question.
If you require adjustments to these APIs, please request precisely the changes you would like to see so that I can make them.
Thank you.
| Microsoft.ApplicationInsights.Metric.TryGetDataSeries(out Microsoft.ApplicationInsights.Metrics.MetricSeries series, bool createIfNotExists, params string[] dimensionValues) -> bool | ||
| Microsoft.ApplicationInsights.Metric.TryGetDataSeries(out Microsoft.ApplicationInsights.Metrics.MetricSeries series, string dimension1Value) -> bool | ||
| Microsoft.ApplicationInsights.Metric.TryGetDataSeries(out Microsoft.ApplicationInsights.Metrics.MetricSeries series, string dimension1Value, string dimension2Value) -> bool |
There was a problem hiding this comment.
why do we need Try alternative? Why not limit it to unconditional method? What customer will do when it returned false?
There was a problem hiding this comment.
Thank you for this question.
The Try-pattern is employed here in-line with the Framework Design Guidelines, because the method for tracking a value may fail even if the user specifies valid parameters under non-trivial conditions:
A key goal of the metric APIs is to ensure that a service cannot fail due to mistakes in tracing and telemetry collection (nothing is absolute, so we strive for "unlikely to fail"). This includes preventing memory starvation due to unintentional combinatorial explosion in the number if time series.
For that a MetricConfiguration has options for SeriesCountLimit and ValuesPerDimensionLimit. If these limits are reached, but a TryTrackValue(..) call would result in the creation of a new time series, the call will return false and the value will not be tracked.
There are different approaches for handling dimension capping, so rather than prescribing the customer a specific approach, we make it easy to detect this case and to react to the circumstance. Please let me know if you would like me to discuss some of these approaches.
Notably, we do not offer exception-throwing TrackValue(..) equivalents to all TryTrackValue(..) methods. This is because appropriate usage of such TrackValue(..) methods would require them to always be used in a try-catch block. This is not appropriate for tracing and telemetry collection APIs, which should be easy to incorporate in existing code and must not break existing code.
Please let me know if this answers your question.
If you require adjustments to these APIs, please request precisely the changes you would like to see so that I can make them.
Thank you.
| Microsoft.ApplicationInsights.Metrics.MetricManager.MetricManager(Microsoft.ApplicationInsights.Metrics.Extensibility.IMetricTelemetryPipeline telemetryPipeline) -> void | ||
| Microsoft.ApplicationInsights.Metrics.MetricManager.Metrics.get -> Microsoft.ApplicationInsights.Metrics.MetricsCollection | ||
| Microsoft.ApplicationInsights.Metrics.MetricSeries |
There was a problem hiding this comment.
do we need to expose it? I haven't looked at it yet, but curious. I understand there is a Metric that you get via GetMetric, aggregator that you can configure for customization. What else do we need?
There was a problem hiding this comment.
I apologize, but I am not sure what you mean by "it" in your question "do we need to expose it?".
The GitHub review tool more than 1 API to which this may refer. I will address your question to the best of my ability. Please let me know if you referred to a different API.
- The read-only
Metricsproperty of theMicrosoft.ApplicationInsights.Metrics.MetricManagerclass:
This provides access to all metrics in the manager's scope. A key use case is to allow users to manage existing metrics. For instance, consider an long-running application that manages containers with a limited life-time and tracks metrics about those containers. When a container life-time has finished, the business system can be sure that no further metrics about this container will be sent. However, the telemetry subsystem does not have this information. The Metrics collection allows a user to release no longer needed metrics and free up the corresponding memory.
- The
Microsoft.ApplicationInsights.Metrics.MetricSeriesclass:
One of the primary requirements for the metrics APIs is to allow tracking 1000s of metric values per second with a small performance overhead. This requires the ability to obtain a handle to a specific metric series and to cache a reference to it so that it can be used to track values without the need for further look-ups. The MetricSeries class provides APIs for doing this without the need for any look-ups or object allocations.
This technique is discussed in detail in the samples 2 and 2a:
https://github.com/Microsoft/ApplicationInsights-dotnet/blob/feature/PreAggMetrics.Master/Test/Microsoft.ApplicationInsights.Test/Shared/Metrics/MetricsExamples.cs
Please let me know if this answers your question.
If you require adjustments to these APIs, please request precisely the changes you would like to see so that I can make them.
Thank you.
| const Microsoft.ApplicationInsights.MetricDimensionNames.TelemetryContext.Cloud.RoleName = "TelemetryContext.Cloud.RoleName" -> string | ||
| const Microsoft.ApplicationInsights.MetricDimensionNames.TelemetryContext.Component.Version = "TelemetryContext.Component.Version" -> string | ||
| const Microsoft.ApplicationInsights.MetricDimensionNames.TelemetryContext.Device.Id = "TelemetryContext.Device.Id" -> string |
There was a problem hiding this comment.
why do you need all these constants exposed publicly? Especially I concerned for the deprecated properties like device ID and screen resolution
There was a problem hiding this comment.
Thank you for this question.
The usage, the reason and the background for the constants in MetricDimensionNames are described in detail in the doc comments for that class, as well as in the samples 2 and 5:
We have also previously discussed this, albeit briefly.
Please let me re-iterate some key background points to put the aforementioned resources in context:
A key feedback point for MetricManager v1 was that it did not allow to specify TelemetryContext properties. For example, if other kinds of telemetry are associated with a specific session or a specific operation name, they use the corresponding properties within the TelemetryContext to convey that information. However, metric aggregates produced by MetricManager v1 did allow access to the TelemetryContext, and such information needed to be stored as a normal dimensions. The inability to overcome this inconsistency was named as a release blocker by the SDK team at that time.
Exposing TelemetryContext for metric series remains impractical because series represent aggregation scopes and require immutable identities that are fast to compare. So, to provide a solution for the described problem, we use well-defined dimension names. Dimensions with such names are treated as normal dimensions during pre-aggregation, but when the resulting aggregates are serialized for transport, the values of such dimensions are stored within the corresponding fields of the TelemetryContext.
Please let me know if this answers your question.
If you require adjustments to these APIs, please request precisely the changes you would like to see so that I can make them. Especially, if certain fields do no longer belong into the TelemetryContext and should be treated as normal dimensions, we should remove them from the constants list.
Thank you.
| static Microsoft.ApplicationInsights.Metrics.MetricIdentifier.DefaultMetricNamespace.set -> void | ||
| static Microsoft.ApplicationInsights.Metrics.TelemetryConfigurationExtensions.GetMetricManager(this Microsoft.ApplicationInsights.Extensibility.TelemetryConfiguration telemetryPipeline) -> Microsoft.ApplicationInsights.Metrics.MetricManager | ||
| static readonly Microsoft.ApplicationInsights.MetricConfigurations.Common -> Microsoft.ApplicationInsights.MetricConfigurations |
There was a problem hiding this comment.
The review tool shows 3 APIs with return types and parameters including 5 types. Could you please specify what you refer to and what changes, if any, you require?
| override Microsoft.ApplicationInsights.Metrics.MetricConfiguration.Equals(object obj) -> bool | ||
| override Microsoft.ApplicationInsights.Metrics.MetricConfiguration.GetHashCode() -> int | ||
| override Microsoft.ApplicationInsights.Metrics.MetricIdentifier.Equals(object otherObj) -> bool |
There was a problem hiding this comment.
Judging by name MetricIdentifier sounds like too low level of a detail
There was a problem hiding this comment.
Could you please help me understand your question? Do you have concerns with the name "MetricIdentifier"? Do you have concerns with the usage of the type MetricIdentifier? Something else? The level of detail is to low for what specifically? What detail exactly do you refer to?
In order to provide as much context as possible, let me please describe the purpose and the usage of the MetricIdentifier class.
A metric is a grouping of one or more metric data series. Besides the actual data (aka the metric series) such a group has 2 attributes:
i) The metric configuration. It is a set of settings that define the behavior of the group and the contained data. For example, it specifies how values are aggregated, how many series are allowed in the group and so on. Metrics with the same configuration may contain different data, but they will exhibit the same behavior. You can think of this as loosely corresponding to a "class type" for a metric.
ii) The metric identifier. It is a set of settings that define the identity of the group so that it can be differentiated from any other groups (aka from other metrics). The identifier includes the metric name, the metric namespace, as well as the number and the names of the metric dimensions. An identifier uniquely identifies a metric within the respective scope. You can think of this as loosely corresponding to a "class instance id" of a metric.
Please let me know if this answers your question.
If you require adjustments to these APIs, please request precisely the changes you would like to see so that I can make them.
Thank you.
| static Microsoft.ApplicationInsights.Metrics.TelemetryConfigurationExtensions.GetMetricManager(this Microsoft.ApplicationInsights.Extensibility.TelemetryConfiguration telemetryPipeline) -> Microsoft.ApplicationInsights.Metrics.MetricManager | ||
| static readonly Microsoft.ApplicationInsights.MetricConfigurations.Common -> Microsoft.ApplicationInsights.MetricConfigurations | ||
| virtual Microsoft.ApplicationInsights.Metrics.MetricConfiguration.Equals(Microsoft.ApplicationInsights.Metrics.MetricConfiguration other) -> bool |
There was a problem hiding this comment.
MetricConfiguration namespace looks empty - only two methods?
There was a problem hiding this comment.
I am not sure what you mean by this comment. Please let me make some educated guesses about what you mean and try to provide as much information as possible.
MetricConfiguration namespace looks empty - only two methods?
-
There is no namespace
MetricConfiguration. There is a type with this name, but it has more than two members. I have described the usage and the purpose of that class in great details just a bit above in the response to your comment aboutMetricIdentifier. -
There is also an identifier with a similar name
MetricConfigurations. Is is also not a namespace, but a static class. Its usage and purpose is described in the doc comments and in examples 1, 2, 3 and 5:
https://github.com/Microsoft/ApplicationInsights-dotnet/blob/feature/PreAggMetrics.Master/Test/Microsoft.ApplicationInsights.Test/Shared/Metrics/MetricsExamples.cs
Please let me provide additional details here.
MetricConfigurations represents an easily discoverable (via Intellisense) point for pre-defined metric configurations:
The common TelemetryClient.GetMetric(..) method has several overloads expecting a parameter of type MetricConfiguration. When attempting to type this term, the user discovers the static instance MetricConfigurations.Common (in the same namespace). We use extension methods to define common metric configurations:
In this version, the base SDK defines MetricConfigurations.Common.Measurement().
The Metrics.Extensions package is planned to add:
MetricConfigurations.Common.Accumulator()MetricConfigurations.Common.Gauge()MetricConfigurations.Common.NaiveDistinctCount()
You have seen the complete ready and tested code for those additional configurations and you requested that we move it to a different repo. It is currently available in the SDK Labs Repo.
Users may also add their own common configurations via the same extension method mechanism.
Please let me know if this answers your question.
If you require adjustments to these APIs, please request precisely the changes you would like to see so that I can make them.
Thank you.
Support W3C incoming headers in opt-in mode
Hi folks,
This is the Merge PR for Metrics Pre-Aggregation. We have worked on the architecture and APIs previously.
E.g.: #643
The amount of code so substantial that making it fit the folder structure and other conventions may take a couple of iterations. I want to start early and obtain early feedback. I'll be reaching out to people directly for specific feedback questions.