Add Namespace to Metrics.

[macrogreg]

Application Insights custom metrics are currently stored in a backend repository optimized for storing structured events from where they are made available for various user experiences in the Azure Portal (Ibiza), in the Analytics Portal and via the DRAFT APIs. These backends identify a metric within the scope of a resource by a 1-tuple [Metric Name]. In the future, custom metrics will be primarily stored in the Geneva pipeline (a backend repository that is optimized for storing timeseries) and made available via the Azure Monitoring APIs [https://docs.microsoft.com/en-us/rest/api/monitor/metrics/list]. Those backends identify a metric within the scope of a resource by a 2-tuple [Metric Namespace, Metric Name]. In order to support these new backends we need to add namespace as an additional attribute to Metric Telemetry Items as a sibling to name wherever name appears. Also, need to add API overloads that create metric items while specifying namespace.

Specifically:

1. Add an attribute metricNamespace to the bond spec of DataPoint next to name. (We call it metricNamespace rather than namespace to avoid clashed with the frequently used keyword “namespace”)

2. Add a property MetricNamespace to the MetricTelemetry class.

3. For each ctor of MetricTelemetry class that includes a name parameter, add an overload that also expects a parameter metricNamespace.

4. Add an Obsolete Attribute to each ctor of the MetricTelemetry class that includes a name parameter, but not a metricNamespace parameter.

5. Add / update unit tests according to the above changes.

Thank you, all, for your feedback about these upcoming changes!

[Dmitry-Matveev]

metricNamespace is an optional attribute in the schema based on the PR above, will it be also optional in the new API to create MetricTelemetry?

In case it's optional, it should not be an issue to mark old methods as obsolete unless behavior will change as well even with empty namespace. This will simplify initial migration. I'll let @SergeyKanzhelev to jump in on this if he disagrees :).

In case it's not optional (or maybe, in any case) we'd also need a good (but short) guidance for those unfamiliar with namespaces in metric telemetry. My guess is that larger part of the existing SDK customers simply used name with SDK metrics for a while without need or wonder about namespaces. Some good IntelliSense hints may help here.

Overall, looks good to me.

[macrogreg]

Thanks, @Dmitry-Matveev , I think you bring up a good point. namespaces must be optional in the schema because older SDKs do know know about it. The ingestion endpoint will set it to a non-empty default value if it is missing or empty. In the SDK we can go with both options: a) We can deprecate overloads that do not have namespace, essentially forcing customers to think about it. b) We can leave those overloads there. Although I do not feel strongly about the choice, the reason I prefer the deprecation is the following:

Previously, metrics have been sent by creating metric telemetry items more or less directly: either via the ctor or via the TrackMetric(..) methods, which we are now deprecating (see https://github.com/Microsoft/ApplicationInsights-dotnet/issues/760). As a result, now, Metric Telemetry should no longer be created directly, and pre-aggregating APIs should be used instead. Therefore, MetricTelemetry is essentially a wire protocol class. It does not need to have convenience ctors. The few advanced users who want to build their own pre-aggregation subsystem that eventually serializes to MetricTelemetry should see the ctor that exposes all of the fields that should be provided. Makes sense? :)

[SergeyKanzhelev]

Add an attribute metricNamespace to the bond spec of DataPoint next to name. (We call it metricNamespace rather than namespace to avoid clashed with the frequently used keyword “namespace”)

Can you please clarify how this will clash?

@macrogreg I agree with @Dmitry-Matveev that obsoleting something that we expect would be used is not a great user experience. What is more desirable for the best user experience - user specifying many semi-random namespaces or all metrics end up in default namespace by default? My understanding is that second is more desirable as it will make UI cleaner. So the overload with namespace is a next step after using one without it.

[macrogreg]

Add an attribute metricNamespace to the bond spec of DataPoint next to name. (We call it metricNamespace rather than namespace to avoid clashed with the frequently used keyword “namespace”)

Can you please clarify how this will clash?

"namespace" is a reserved keyword in several languages, first of all in C#. You cannot have variables or parameters with that name. It is also a keyword in PHP, C++, TypeScript, and whatever languages I cannot think of right now. Naming a cross-platform variable that way would be a bad practice.

What is more desirable for the best user experience - user specifying many semi-random namespaces or all metrics end up in default namespace by default? My understanding is that second is more desirable as it will make UI cleaner. So the over load with namespace is a next step after using one without it.

The key point is that people should now be using GetMetric(..) APIs to track metrics. Those APIs do have overloads with or without namespaces. The argument that people would be specifying semi-random namespaces is not correct: It would be the case if people would be creating MetricTelemetry objects themselves. But that is exactly what we want to discourage!

I am perfectly happy to not deprecate the MetricTelemetry overloads that omit namespaces if that is what you prefer. Consider it done: Point (4) from the original description is cut. My key point is different: We shipped the MetricManager / GetMetric APIs, but your guidance seems to be not to use them: Continue creating MetricTelemetry explicitly, do not deprecate TrackMetric (https://github.com/Microsoft/ApplicationInsights-dotnet/issues/760) and so on. We are pulling in different directions. How can we realign and work towards success here?

[SergeyKanzhelev]

how the field in schema and property name may clash with the keyword? Will C# or PHP forbid to use the field with this name?

We are pulling in different directions. How can we realign and work towards success here?

No. I don't see it this way. We have two features - track metric as we track all other data types. And pre-aggregate metrics. I want to make sure we have clearly defined way to convert metrics aggregated by other providers into the Metric Telemetry recognizable by Azure Monitor.

I will encourage people to use pre-aggregated metrics. So I'm with you here. I just want to keep lights on for users who need a different feature of our SDK.

[macrogreg]

how the field in schema and property name may clash with the keyword? Will C# or PHP forbid to use the field with this name?

In most cases public "property-style" APIs match the bond names. Indeed, it is nice when things are consistent and the property name in json (or other protocol) is the same as the property name in the wrapping public SDK type, is the same as the ctor parameter name and so on and so on. For that it needs not to clash with reserved keywords.

That being said, I do not think this is a worthwhile discussion. If you feel that metricNamespace is not the ideal moniker, please let me know precisely what you prefer to see in its place and I'll make it happen.

[macrogreg]

I want to make sure we have clearly defined way to convert metrics aggregated by other providers into the Metric Telemetry recognizable by Azure Monitor.

I completely agree. That is why MetricTelemetry must remain a public type. But when discussing its specific APIs (ctors and methods), should we not then optimize for people who just built their own aggregation and are in the process of marshaling the result and not for people who just want to send a metric real quick?

Either way, this particular branch of discussion is not really applicable here any longer: As already mentioned, I can just leave the namespace-less overloads as they are.

This branch of the discussion is, however, applicable to https://github.com/Microsoft/ApplicationInsights-dotnet/issues/760, as that issue discusses exactly that: Refocusing MetricTelemetry on being the result of an aggregation, and pointing users to the aggregating APIs.

[SergeyKanzhelev]

Is the word namesapce already exposed in Azure Monitor? If so - I'd suggest keep it short as namespace in schema and as a property on type. Argument names will have longer names. I think final customer bill is more important than convenience of argument name.

If you are still deciding - tags, type or label may be used to align with different big metrics players. Those are shorter and don't have the above mentioned problem of clash with reserved keyword.

For the deprecation discussion I agree that we have a separate thread.

[macrogreg]

@SergeyKanzhelev , please approve https://github.com/Microsoft/ApplicationInsights-Home/pull/207.

[macrogreg]

Hey @SergeyKanzhelev , I just merged the PR to rename metricNamepace to metric. Thanks for approving it. However, I should have followed my own suggestion and built the Bond, before merging. Now, that I did build it, looks like namespace is not an acceptable ID after all:

PS C:\00\Code\GitHub\AI-DotNet\ApplicationInsights-dotnet\Schema> .\generateSchema.ps1

Directory: C:\00\Code\GitHub\AI-DotNet\ApplicationInsights-dotnet\Schema

Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d-----        4/13/2018     15:00                PublicSchema
d-----        4/10/2018     15:56                obj
Attempting to resolve dependency 'Bond.Core.CSharp (= 4.2.1)'.
Attempting to resolve dependency 'Bond.Runtime.CSharp (= 4.2.1)'.
Attempting to resolve dependency 'Newtonsoft.Json (≥ 7.0.1)'.
'Bond.CSharp 4.2.1' already installed.
C:\00\Code\GitHub\AI-DotNet\ApplicationInsights-dotnet\Schema\PublicSchema\DataPoint.bond(10,37) : error B0000: unexpected reserved word "namespace", expecting letter or digit or "_"
C:\00\Code\GitHub\AI-DotNet\ApplicationInsights-dotnet\Schema\PublicSchema\DataPoint.bond(10,37) : error B0000: unexpected reserved word "namespace", expecting letter or digit or "_"

If you feel that metricNamespace is too long, we can pick something shorter for sure. Please let me know.

[macrogreg]

Maybe metricNs?

[SergeyKanzhelev]

Perhaps just ns. And in C# use the full name.

I suggested other alternatives. But those will only work if Azure Monitor don't have the term locked yet

Rant start

why bond has this limitation at all?

Rant end

[SergeyKanzhelev]

https://github.com/Microsoft/bond/issues/852

[macrogreg]

Thanks for bringing this question up with the bond folks. However, we need to proceed in the meantime. From the above, I see that NS is your preference. I do not mind. PR coming in a few mins.

[macrogreg]

@SergeyKanzhelev : https://github.com/Microsoft/ApplicationInsights-Home/pull/208 :)

[macrogreg]

This time it worked. ns in bond, metricNamespace in the API.

[TimothyMothra]

@macrogreg can this be closed? I see 3 merged PRs.

[macrogreg]

Yes. In the future, should I proactively close Design-Discussion Issues after PRs are merged?

[TimothyMothra]

That would help :)

This week we're reviewing all open items and closing or assigning to milestones. If this discussion still has pending work, feel free to open additional issues linked to this.