Updated ThreadTrackingStatistic to use PerfCounterTelemetryConsumer #2147
Conversation
|
CC: @jason-bragg / @jdom |
| @@ -163,7 +162,7 @@ public void OnStartProcessing() | |||
| firstStart = false; | |||
| if (StatisticsCollector.CollectContextSwitchesStats) | |||
| { | |||
| TrackContextSwitches(); | |||
| logger.IncrementMetric(metricName); | |||
There was a problem hiding this comment.
I don't think this is a good way to trigger logging these metrics. In general, all we are doing here is telling the telemetry that there is a metric of this name that is being incremented, which is not the case. We are then relying on the Orleans perf counter metric consumer to detect this metric and perform special processing on it. This is quite kludgy.
The problem is that we don't have anything to log here, we just need to signal something else to retrieve and log the data, so this is really a signal to an framework extension point. I suspect that DI could help.
Suggest something like:
- Create public IThreadTrackingStatistic interface with the public surface of ThreadTrackingStatistic.
- Move ThreadTrackingStatistic to OrleansTelemetryConsumers.Counters as an implementation of IThreadTrackingStatistic.
- Add extension function to register ThreadTrackingStatistic type in the service collection for DI.
4 Update Orleans to get IThreadTrackingStatistic form DI.
There was a problem hiding this comment.
I second your coment. What will happen if I implement another telemetry consumer ? I will get just a single call to Context Switches/sec, what is it means for me ?
Obviosly telemetry data coming into interface should make sense to all consumers and not to be custom fit for one specific implementation.
It just one specific example of a more general leaking abstraction in design of IMetricTelemetryConsumer. Orleans has many metrics constructed from other metrics but IMetricTelemetryConsumer don't have any general interface to implement this.
(will probably be resolved by dotnet#2147)
|
Hey guys, this PR is outdated with the lots of changes recently made by @jason-bragg Should we close it? |
|
Yeah, I've a work item to create a tracking issue for this and close it out. I'll do that today. |
|
Closing as alternative solutions are preferred and PR is out of date. |
More work for #2122