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

Deprecate non-preaggregating Metric APIs #760

Closed
macrogreg opened this issue Apr 10, 2018 · 3 comments
Closed

Deprecate non-preaggregating Metric APIs #760

macrogreg opened this issue Apr 10, 2018 · 3 comments

Comments

@macrogreg
Copy link
Contributor

macrogreg commented Apr 10, 2018

Metrics should always be pre-aggregated across a time period before being sent.

Applications emit metrics when they observe numerical values, however, the user is not usually interested in every single value, but instead in a statistical summary over some time period (e.g., the average measurement value over a minute interval). Applications Insights and Azure Monitoring uses Metric Telemetry data items to support this use case. Azure Monitoring APIs are always aggregating. Metric values are always aggregated and queries are only possible for specific time buckets and not for specific values tracked originally.

On contrary, Applications that are interested in actually observed values, without aggregation across time, need to be using Event Telemetry data items. Such items contain fields for custom measurements. These measurements are available for analytical queries, plots and the-like, but they are not available as Azure Monitor metrics by default.

In a recent SDK Beta we shipped APIs that support metric pre-aggregation by default. Users can track individual values, potentially many thousand times per second. The values are not sent immediately, but aggregated into aggregates, containing statistical summaries over time periods. These aggregates are sent at regular time intervals.

The Application Insights SDK also contains legacy APIs that incorrectly encourage users to misuse the above application model and to send non-aggregated metric values to the ingestion endpoints. Now, we want to deprecate such APIs:

  • MetricTelemetry ctors that take only a single metric value will be deprecated. Users should use TelemetryClient.GetMetric(..) and Metric.[Try]TrackValue(..) overloads to track values. (However, some expert users may choose to implement their own pre-aggregation subsystems. To support such cases, the MetricTelemetry ctors that takes statistical summary values remain publicly available.)

  • TelemetryClient.TrackMetric(..) overloads will be deprecated. Existing application code that uses those methods will receive warnings during recompilation. Such warnings will contain descriptive messages on transitioning to the GetMetric(..) APIs. The few expert users who choose to implement their own aggregation subsystems may use the unspecific TelemetryClient.Track(ITelemetry) method to track their custom aggregates.

  • Update documentation. Specifically, https://docs.microsoft.com/en-us/azure/application-insights/app-insights-api-custom-events-metrics#trackmetric. (What else?)

We have previously discussed these changes at length and agreed on this strategy with the SDK architects and the Metrics work group. This atricle is to facilitate a broader discussion before proceeding with the changes. In particular, the changes will ensure:

  1. That new Metric Pre-Aggregation APIs can be discovered by removing potential confusion with the legacy TrackMetric(..) methods.
  2. That customers can discover inappropriate TrackMetric(..) uses and that a helpful resolution message is provided.
  3. That advanced “Einstein” customers and monitoring solution providers are supported in implementing their own aggregation logic.

Thank you for your feedback.

@macrogreg
Copy link
Contributor Author

macrogreg commented Apr 12, 2018

Not much feedback to date. This may be because I published this article not so long ago, or because we have already discussed this internally. I will wait a little longer to make sure everybody has enough time to provide feedback. In the meantime, I published a PR: #767

That PR makes code changes described here (except the public website doc changes which are not in this repo). We can update the PR as the discussion here progresses. However, let us please be mindful of the release timelines.

@SergeyKanzhelev
Copy link
Contributor

Sorry for delay. Copying my reply from PR below. Just to summarize - I believe making this change now will make more harm than good. We want to keep a way for advanced users to send MetricTelemetry. So we need corresponding Track method for consistency.

This API method is align with other Track methods. It provides a short alternative to the method that accepts MetricTelemetry. If we agree second method should stay - this method is just a shortcut for it.
Another consideration - this method was promoted in many examples and documentation. It also has the same signature in other languages. It may be very expensive to change the signature everywhere.
I agree with the idea that TackMetric as a single value metric uploader will not be aligned with the Application Insights data model going forward. Application Insights is moving away from storing metrics in Analytics portal in favor of metric-specific storage. With the change of the data model semantic I believe it may be logical to change SDK as well.
So if only aggregated metrics will be shown in UI for TrackMetric call - perhaps this call should not send even immidiately and opt into the different behavior.
Same as TrackRequest does not guarantee that request will be sent. It only guarantees that request metadata will be used for pre-aggregaterd metrics and request will be sent if it represent "sampled" data item.

All these said I'd suggest to go with the change of semantics of a call once we'll make this change in back-end and re-route metrics from Analytics to Metrics storage.

@TimothyMothra TimothyMothra added this to the 2.7 milestone Apr 20, 2018
@cijothomas cijothomas modified the milestones: 2.7, 2.9 Oct 9, 2018
@cijothomas cijothomas modified the milestones: 2.9, 2.10 Feb 21, 2019
TimothyMothra pushed a commit that referenced this issue Oct 25, 2019
Update to 2.8.0-beta2 of base wek. Bump version to 2.5.0-beta2
@github-actions
Copy link

This issue is stale because it has been open 300 days with no activity. Remove stale label or comment or this will be closed in 7 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants