add Terminal service & RemoteFx Collector #491

asiyani · 2020-03-29T18:47:28Z

Added collector for

Terminal service performance matrics
Win32_PerfRawData_LocalSessionManager_TerminalServices & Win32_PerfRawData_TermService_TerminalServicesSession
Connection Broker Performance matrics, Total session counts in RDS farm (Only if host is a connection Broker)
Win32_PerfRawData_RemoteDesktopConnectionBrokerPerformanceCounterProvider_RemoteDesktopConnectionBrokerCounterset
RemoteFx Network and Graphics Performance matrics
Win32_PerfRawData_Counters_RemoteFXNetwork & Win32_PerfRawData_Counters_RemoteFXGraphics metrics

breed808 · 2020-03-29T23:44:37Z

Nice! I'm especially looking forward to using the terminal services collector at work.

From a brief look at the metrics available in perflib, I think the session metrics could be queried via perflib rather than WMI (though not the wmi_terminal_services_connection_broker_performance_total metric). It looks like the Remote FX Graphics/Network metrics can also be queried via perflib.

asiyani · 2020-03-30T16:19:21Z

I am making required changed to use perflib.
just courius why perflib is prefered over wmi?

breed808 · 2020-03-30T21:30:35Z

The preference to use perflib is due to performance and stability reasons. WMI calls are expensive, and sometimes return inconsistent metric data (#89 is a good example).
We should document that perflib is the preferred metric source in the docs or a CONTRIBUTING.md template.

breed808

Looking good! I've added a few comments for some minor changes.
Let me know if you need any clarification.

breed808 · 2020-03-31T11:24:25Z

collector/remote_fx.go

+		ch <- prometheus.MustNewConstMetric(
+			c.BaseTCPRTT,
+			prometheus.GaugeValue,
+			float64(d.BaseTCPRTT),


You can remove the float64() cast on all metrics as the perflib metrics are already defined as float64 in perflibRemoteFxNetwork and perflibRemoteFxGraphics.

breed808 · 2020-03-31T11:27:29Z

collector/remote_fx.go

+			d.Name,
+		)
+		ch <- prometheus.MustNewConstMetric(
+			c.TCPReceivedRate,


I've run the exporter on my local machine, and a number of the metrics are monotonically increasing, and should be changed to CounterValue rather than the current GaugeValue.
I noted that this was the case for net_total_received_rate, net_total_sent_rate, tcp_sent_rate, tcp_received_rate, net_total_sent_bytes and net_total_received_bytes. There's likely more metrics than need to be changed.

I need to investigate this further but when i checked not all counters are increasing. (exceptions are services and console sessions. Since these 2 are not actually remote sessions I have added filters for them. )
I will do further testing on this my side. If you have some time I whould appreciate if you can help me with this.

breed808 · 2020-03-31T11:28:23Z

collector/remote_fx.go

+		//gfx
+		AverageEncodingTime: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "gfx_average_encoding_time"),
+			"Average frame encoding time.",


What unit does this metric represent? Seconds? Milliseconds?

Milliseconds updating help.

breed808 · 2020-03-31T11:30:35Z

collector/remote_fx.go

+		),
+		FECRate: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "net_fec_rate"),
+			"Forward Error Correction (FEC) percentage",


Is this an error percentage for all traffic? I'm not sure what this metric is representing.

documentation is not clear but all these counters are /session

breed808 · 2020-03-31T11:31:21Z

collector/remote_fx.go

+		),
+		LossRate: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "net_loss_rate"),
+			"Loss percentage",


Is this a loss percentage for all traffic in the session?

documentation is not clear but all these counters are /session

breed808 · 2020-03-31T11:38:08Z

docs/collector.terminal_services.md

+|||
+-|-
+Metric name prefix  | `terminal_services`
+Data source         | Perflib


Will need to be Perflib/WMI as both sources are queried for this collector.

breed808 · 2020-03-31T11:39:15Z

collector/terminal_services.go

+		ch <- prometheus.MustNewConstMetric(
+			c.HandleCount,
+			prometheus.GaugeValue,
+			float64(d.HandleCount),


Redundant float64() cast can be removed, as variables are initialized as float64.

breed808 · 2020-04-01T04:22:50Z

@asiyani thanks for the changes! I'll investigate further on which metrics should be changed to counters. I'll try to compile a list of metrics that can then be reviewed by others.

asiyani · 2020-04-01T10:29:46Z

Further investigation on counters. I will change counter type to following values. Please let me know if you find it different in your testing.

wmi_remote_fx_gfx_average_encoding_time | gauge - fluctuating values
wmi_remote_fx_gfx_frame_quality | gauge - fluctuating values
wmi_remote_fx_gfx_frames_skipped_persec_insufficient_clt_res | counter
wmi_remote_fx_gfx_frames_skipped_persec_insufficient_net_res | counter
wmi_remote_fx_gfx_frames_skipped_persec_insufficient_srv_res | counter
wmi_remote_fx_gfx_graphics_compression_ratio | gauge - fluctuating values
wmi_remote_fx_gfx_input_frames_persec | Counter
wmi_remote_fx_gfx_output_frames_persec | Counter
wmi_remote_fx_gfx_source_frames_persec | Counter

wmi_remote_fx_net_base_udp_rrt | unknown (should be gauge to match tcp rrt)
wmi_remote_fx_net_base_tcp_rrt | gauge - fluctuating values
wmi_remote_fx_net_current_tcp_bandwidth | gauge - fluctuating values
wmi_remote_fx_net_current_udp_bandwidth | unknown - (should be gauge match tcp bw)
wmi_remote_fx_net_current_tcp_rtt | gauge - fluctuating values
wmi_remote_fx_net_current_udp_rtt | unknown - values are zero ( should be same as tcp_rtt)
wmi_remote_fx_net_fec_rate | unknown - values are zero ( should be Counter match other rate values)
wmi_remote_fx_net_loss_rate | unknown - values are zero ( should be Counter match other rate values)
wmi_remote_fx_net_retransmission_rate | unknown - values are zero ( should be Counter match other rate values)
wmi_remote_fx_net_tcp_received_rate | Counter
wmi_remote_fx_net_tcp_sent_rate | Counter
wmi_remote_fx_net_total_received_rate | Counter
wmi_remote_fx_net_total_sent_rate | Counter
wmi_remote_fx_net_total_received_bytes | Counter
wmi_remote_fx_net_total_sent_bytes | Counter
wmi_remote_fx_net_udp_packets_received_persec | Counter
wmi_remote_fx_net_udp_packets_sent_persec | Counter
wmi_remote_fx_net_udp_received_rate | Counter
wmi_remote_fx_net_udp_sent_rate | Counter

wmi_terminal_services_local_session_count | Gauge
wmi_terminal_services_connection_broker_performance_total| counter
wmi_terminal_services_handle_count | Gauge
wmi_terminal_services_page_fault_per_sec | Gauge
wmi_terminal_services_page_file_bytes | Gauge
wmi_terminal_services_page_file_bytes_peak | Gauge
wmi_terminal_services_percent_privileged_time | Counter
wmi_terminal_services_percent_processor_time | Counter
wmi_terminal_services_percent_user_time | Counter
wmi_terminal_services_pool_non_paged_Bytes | Gauge
wmi_terminal_services_pool_paged_bytes | Gauge
wmi_terminal_services_private_bytes | Gauge
wmi_terminal_services_thread_count | Gauge
wmi_terminal_services_virtual_bytes | Gauge??
wmi_terminal_services_virtual_bytes_peak | Gauge??
wmi_terminal_services_workingset | Gauge
wmi_terminal_services_workingset_peak | Gauge ??

breed808 · 2020-04-01T22:33:11Z

I've done some testing and can clarify some of the metrics:

I'm uncertain of wmi_remote_fx_net_fec_rate, wmi_remote_fx_net_loss_rate and wmi_remote_fx_net_retransmission_rate as I can't get any data for these metrics on my testing machine. From the name I assume they are counters, as perflib generally exposes a counter for any metric containing "rate".

@asiyani the only change left to make would be to rename the counter metrics. E.G. change wmi_remote_fx_gfx_output_frames_persec to wmi_remote_fx_gfx_output_frames_total so it's clear this metric is a counter.

@carlpett this looks almost complete, did you want to review?

asiyani · 2020-04-06T07:59:23Z

@breed808 when you got time please review this last change and let me know if any other issues.
Thank you

breed808

Looks good!

carlpett · 2020-04-07T18:54:38Z

Thank you for this work @asiyani, really nice! And thanks @breed808 for reviewing! Apologies for slow reactions from me.
I think this looks mostly good to go, just some naming things that would be good to fix:

Some metrics have but others do not have units (ie missing _bytes or similar), and not all are in base form (milliseconds instead of seconds). Could we harmonize all of that? (See upstream best practices for more details)
There are a couple of percent or rate metrics - are those actually reporting percentages and rates? Those is very often named for how they are intended to be presented after calculations, not what the actual data is.

asiyani · 2020-04-13T14:42:37Z

Regarding _Rate in remote_fx_net most of the _Rate are zero values. I am thinking of removing those and just keeping rtt, bandwidth and followings.
wmi_remote_fx_net_total_received_bytes
wmi_remote_fx_net_total_sent_bytes
wmi_remote_fx_net_udp_packets_received_total
wmi_remote_fx_net_udp_packets_sent_total

This will remove confusion about _rate and from _total we can calculate rate.
what you think? @breed808 @carlpett

carlpett · 2020-04-13T18:55:35Z

Nice! 👏
Agree, removing the _rate and keeping the _total sounds like a good path forward 👍

asiyani · 2020-04-14T08:17:28Z

_rate metrics are removed from remote fx.
Let me know if there is any other changes needs to be done.

asiyani · 2020-04-19T12:13:23Z

@carlpett when you have a time, Please review this for me.

carlpett

Hey,
Sorry, bit of a lack of time this week.
I took another look now, and came upon some things I missed last round. Mainly naming and units, tried to leave suggestions for those so you can hopefully batch that up to skip the more tedious parts if you agree with them.

carlpett · 2020-04-19T19:15:49Z

collector/remote_fx.go

+			nil,
+		),
+		TotalReceivedBytes: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "net_total_received_bytes"),


Somewhat nitty, but by convention the _total should be at the end

Suggested change

prometheus.BuildFQName(Namespace, subsystem, "net_total_received_bytes"),

prometheus.BuildFQName(Namespace, subsystem, "net_received_bytes_total"),

carlpett · 2020-04-19T19:16:01Z

collector/remote_fx.go

+			nil,
+		),
+		TotalSentBytes: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "net_total_sent_bytes"),


Move _total here too

Suggested change

prometheus.BuildFQName(Namespace, subsystem, "net_total_sent_bytes"),

prometheus.BuildFQName(Namespace, subsystem, "net_sent_bytes_total"),

carlpett · 2020-04-19T19:17:54Z

collector/remote_fx.go

+
+		//gfx
+		AverageEncodingTime: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "gfx_average_encoding_time"),


Should have the unit as a suffix

Suggested change

prometheus.BuildFQName(Namespace, subsystem, "gfx_average_encoding_time"),

prometheus.BuildFQName(Namespace, subsystem, "gfx_average_encoding_time_seconds"),

Are we sure the raw data is actually the average and not a counter of total seconds, by the way?

yes, its a GaugeValue

carlpett · 2020-04-19T19:21:21Z

collector/terminal_services.go

+			nil,
+		),
+		PageFaultsPersec: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "page_fault_per_sec"),


Is this actually per_sec or is the raw data a counter?

Yes it looks like total and counter. i will change it.

carlpett · 2020-04-19T19:22:28Z

collector/terminal_services.go

+			nil,
+		),
+		PercentPrivilegedTime: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "privileged_time_total"),


Per the conventions, we should have the unit here as well

Suggested change

prometheus.BuildFQName(Namespace, subsystem, "privileged_time_total"),

prometheus.BuildFQName(Namespace, subsystem, "privileged_time_seconds_total"),

carlpett · 2020-04-19T19:28:18Z

collector/remote_fx.go

+	return &RemoteFxCollector{
+		// net
+		BaseTCPRTT: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "net_base_tcp_rtt"),


Missing a unit

Suggested change

prometheus.BuildFQName(Namespace, subsystem, "net_base_tcp_rtt"),

prometheus.BuildFQName(Namespace, subsystem, "net_base_tcp_rtt_seconds"),

carlpett · 2020-04-19T19:28:27Z

collector/remote_fx.go

+			nil,
+		),
+		BaseUDPRTT: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "net_base_udp_rtt"),


Suggested change

prometheus.BuildFQName(Namespace, subsystem, "net_base_udp_rtt"),

prometheus.BuildFQName(Namespace, subsystem, "net_base_udp_rtt_seconds"),

carlpett · 2020-04-19T19:28:39Z

collector/remote_fx.go

+			nil,
+		),
+		CurrentTCPRTT: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "net_current_tcp_rtt"),


Suggested change

prometheus.BuildFQName(Namespace, subsystem, "net_current_tcp_rtt"),

prometheus.BuildFQName(Namespace, subsystem, "net_current_tcp_rtt_seconds"),

carlpett · 2020-04-19T19:28:47Z

collector/remote_fx.go

+			nil,
+		),
+		CurrentUDPRTT: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "net_current_udp_rtt"),


Suggested change

prometheus.BuildFQName(Namespace, subsystem, "net_current_udp_rtt"),

prometheus.BuildFQName(Namespace, subsystem, "net_current_udp_rtt_seconds"),

carlpett · 2020-04-19T19:30:08Z

collector/remote_fx.go

+		FramesSkippedPerSecondInsufficientClientResources: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "gfx_frames_skipped_insufficient_clt_res_total"),
+			"Number of frames skipped per second due to insufficient client resources.",
+			[]string{"session_name"},
+			nil,
+		),
+		FramesSkippedPerSecondInsufficientNetworkResources: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "gfx_frames_skipped_insufficient_net_res_total"),
+			"Number of frames skipped per second due to insufficient network resources.",
+			[]string{"session_name"},
+			nil,
+		),
+		FramesSkippedPerSecondInsufficientServerResources: prometheus.NewDesc(
+			prometheus.BuildFQName(Namespace, subsystem, "gfx_frames_skipped_insufficient_srv_res_total"),
+			"Number of frames skipped per second due to insufficient server resources.",
+			[]string{"session_name"},
+			nil,
+		),


Would it make sense to merge these into a gfx_frames_skipped_total with a reason label? From an alerting perspective that might make it easier to write a query?

asiyani · 2020-04-23T08:22:41Z

@carlpett I have made changes as you suggested. Intead of commit suggestions, I have done same changes in last commit as it it requires changes to the docs as well.
Let me know if I missed something.

carlpett · 2020-04-23T10:16:58Z

Fantastic work @asiyani, thank you so very much for your contribution and patience! 🎉

asiyani added 7 commits March 23, 2020 09:40

added terminal services collector for number of session count

d9404c6

added terminal services session matrics

cdee996

added connection broker performance counter

63b3d20

added collector for remoteFx

b57ef7f

changed CB counter to counterValue

287b40f

added terminal services docs

a47b770

added remoteFx docs

24e54dd

converted wmi query to perfLib

94c564f

breed808 requested changes Mar 31, 2020

View reviewed changes

asiyani added 5 commits March 31, 2020 15:40

removed float64 cast

6f15108

minor changes in docs

5a1d525

changed remotefx label from session to session_name to match TS

a050930

added filter for services and console sessions

59c3d84

fixed strings import error

af83a4b

asiyani added 2 commits April 1, 2020 17:51

converted required metrics to Counter from Gauge

d0ee23c

fixed typo with baseTCPRTT and baseUDPRTT

fd39e97

renamed remotefx counter labels to _total

14a761d

breed808 approved these changes Apr 6, 2020

View reviewed changes

asiyani added 2 commits April 13, 2020 15:39

converted millisec to sec

2db60c5

changed time variables to _total to reflect counter

5417830

removed _rate from remote fx

77d5acd

carlpett reviewed Apr 19, 2020

View reviewed changes

changed metrics names to indicate units and combined few metrics

9bcb67c

carlpett merged commit 17324b9 into prometheus-community:master Apr 23, 2020

anubhavg-icpl pushed a commit to anubhavg-icpl/windows_exporter that referenced this pull request Sep 22, 2024

add Terminal service & RemoteFx Collector (prometheus-community#491)

4a767bd

	prometheus.BuildFQName(Namespace, subsystem, "net_total_received_bytes"),
	prometheus.BuildFQName(Namespace, subsystem, "net_received_bytes_total"),

add Terminal service & RemoteFx Collector #491

add Terminal service & RemoteFx Collector #491

Conversation

asiyani commented Mar 29, 2020

breed808 commented Mar 29, 2020

asiyani commented Mar 30, 2020

breed808 commented Mar 30, 2020

breed808 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asiyani Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breed808 commented Apr 1, 2020

asiyani commented Apr 1, 2020 • edited Loading

breed808 commented Apr 1, 2020 • edited Loading

asiyani commented Apr 6, 2020

breed808 left a comment

Choose a reason for hiding this comment

carlpett commented Apr 7, 2020

asiyani commented Apr 13, 2020 • edited Loading

carlpett commented Apr 13, 2020

asiyani commented Apr 14, 2020

asiyani commented Apr 19, 2020

carlpett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asiyani commented Apr 23, 2020

carlpett commented Apr 23, 2020

asiyani Mar 31, 2020 •

edited

Loading

asiyani commented Apr 1, 2020 •

edited

Loading

breed808 commented Apr 1, 2020 •

edited

Loading

asiyani commented Apr 13, 2020 •

edited

Loading