search

microsoft / ApplicationInsights-Go

Microsoft Application Insights SDK for Go
MIT License
157 stars 53 forks source link

Measurements map is not initialized for MetricTelemetry and AggregateMetricTelemetry #12

Closed karolz-ms closed 6 years ago

karolz-ms commented 6 years ago

When NewMetricTelemetry() or NewAggregateMetricTelemetry() is called, the resulting telemetry struct has its Measurements field left un-initialized (nil). This is inconsistent with Properties field and makes coding against metrics harder than it needs to be.

jjjordanmsft commented 6 years ago

You cannot submit Measurements against metric telemetry, which is why it is nil. I figured that was better leaving a map that doesn't get used, misleading a user into thinking they submitted telemetry when they did not.

But really, the issue is that Measurements comes from the "base struct", mostly to keep all the telemetry items looking alike and reduce boilerplate, since it implements most of the Telemetry interface. That is an ugly shortcut, and I should probably fix that. (Telemetry.GetMeasurements would still return nil for metrics).

karolz-ms commented 6 years ago

Well, yes, that would prevent me from barking up the wrong tree and implementing a bunch of code to translate the metric information my service is getting into something that turns out to be unsupported in the current version of the library :-/

But also note that although the .NET SDK base does not expose the ability to send multiple measurements as a single metric record, it seems to be a SDK limitation only and the underlying data contract does have this capability https://github.com/Microsoft/ApplicationInsights-dotnet/blob/7321d2bf7b9c4733fb43b6c186e381863931eb1a/src/Microsoft.ApplicationInsights/DataContracts/MetricTelemetry.cs#L35

This capability is used by the data source I am dealing with, namely, the library that is producing the metrics from a service process is doing a lot of pre-aggregations. For example, for "request_time" metric, it is giving me not only sum, stddev, min, max and #of sambles for a given period, but also p75, p90 and p99 percentiles. I'd really like to be able to pass this info along into AI

@SergeyKanzhelev any thoughts on that?

jjjordanmsft commented 6 years ago

Sorry about that.

The last I heard on the subject is that multiple data points in a metric telemetry are not supported, contrary to what the data contracts say. I don't think the data collector rejects them, but instead it's one of those undefined behavior scenarios. Sergey might correct me here.

It sounds like p75, p90, and p99 can be single value metrics in their own right.

When my team has needed to send multiple metrics that are grouped together -- an example is detailed memory usage information, in which we need to be able to find all the values from the same sample during analysis -- we attach multiple measurements to an Event and access that data via Log Analytics queries rather than the portal.

karolz-ms commented 6 years ago

All right, thanks JJ. I will refrain from sending these values for now.

Feel free to close this issue as necessary, but I am curious what Sergey thinks about it.

jjjordanmsft commented 6 years ago

Closing -- I did mention this to @SergeyKanzhelev a bit ago, and you can follow up with him directly.