-
Notifications
You must be signed in to change notification settings - Fork 94
[dataset] Add dataset "system uptime" into non-db client. #52
Conversation
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
sonic_data_client/non_db_client.go
Outdated
@@ -63,6 +63,10 @@ var ( | |||
path: []string{"OTHERS", "proc", "stat"}, | |||
getFunc: dataGetFunc(getProcStat), | |||
}, | |||
{ // Get host uptime | |||
path: []string{"OTHERS", "platform", "sysuptime"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the path be proc/uptime
?
The other proc
based paths have proc
prefix. This is in the same category with them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the dataset system uptime
is depended on platform, just like the dataset CPU utilization
at line 43. @pra-moh any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yozhao101 "/proc/uptime" should not be dependent on platform. Do you have any reference here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I referred the dataset of CPU utilization
. Did you mean we should put system uptime under the query of proc
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes let's put it as /proc/uptime
prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated! Please have a review!
sonic_data_client/non_db_client.go
Outdated
func getSysUptime() ([]byte, error) { | ||
timeInfo, _ := linuxproc.ReadUptime("/proc/uptime") | ||
uptime := sysUptime{} | ||
uptime.Total = uint64(timeInfo.Total) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can simply marshal and return whatever linuxproc.ReadUptime
gives.
Do we need to convert this to another struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thelinuxproc.ReadUptime("/proc/uptime")
will return a struct which contains two float
values: one value is system uptime, the other is the idle time which all CPUs are in the idle state. I sent an email to Pradyna and Hui. Hui confirmed that currently we are only interested in system uptime
and do not care about idle time
. Also we need convert the float
to int
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yozhao101 can you please show sample output of linuxproc.ReadUptime("/proc/uptime")? I tend to agree with Murat here because we can always discard from collector instead here to avoid any regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pra-moh The following is the sample output of linuxproc.ReadUptime("/proc/uptime"). Please review it.
admin@str-a7050-acs-3:~$ ./gnmi_cli -client_types=gnmi -a localhost:8080 -t OTHERS -logtostderr -insecure -qt p -pi 10s -q platform/sysuptime
{
"OTHERS": {
"platform": {
"sysuptime": "{\"total\":2986.02,\"idle\":10512.98}"
}
}
}
{
"OTHERS": {
"platform": {
"sysuptime": "{\"total\":2996.02,\"idle\":10547.75}"
}
}
}
{
"OTHERS": {
"platform": {
"sysuptime": "{\"total\":3006.02,\"idle\":10583.74}"
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Yong. Let's return total and idle values as they are. As Murat suggested just marshal and return don't do any conversion. We can handle conversion/discarding idle at collector side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will update.
…f CPU utilization. Signed-off-by: Yong Zhao <yozhao@microsoft.com>
long CPUs are in the idle state. Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
sonic_data_client/non_db_client.go
Outdated
@@ -43,6 +43,10 @@ var ( | |||
path: []string{"OTHERS", "platform", "cpu"}, | |||
getFunc: dataGetFunc(getCpuUtil), | |||
}, | |||
{ // Get host uptime | |||
path: []string{"OTHERS", "proc", "sysuptime"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to name this query proc/uptime
(e.g. drop sys
). The other queries in this category follow a particular convention where query path proc/xyz
directly maps to file system /proc/xyz
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! Updated.
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao yozhao@microsoft.com
Motivation of this PR:
This PR aims to enable the streaming telemetry container to stream out the
system uptime
of SONiC Host. This dataset is added into non-database client since it is a kind of value which will be refreshed periodically.How can I do that?
I follow the example of dataset in non-database client such as
meminfo
to do the implementation. The data source ofsystem uptime
is from the file/proc/uptime
on the SONiC host.We can use the command
./gnmi_cli -client_types=gnmi -a <DuT_IP>:8080 -t OTHERS -logtostderr -insecure -qt p -pi 10s -q proc/uptime
to query thesystem uptime
every 10 seconds.admin@str-a7050-acs-3:~$ ./gnmi_cli -client_types=gnmi -a localhost:8080 -t OTHERS -logtostderr -insecure -qt p -pi 10s -q proc/uptime
{
"OTHERS": {
"proc": {
"uptime": "{"total":314.74,"idle":981.27}"
}
}
}
{
"OTHERS": {
"proc": {
"uptime": "{"total":324.74,"idle":1016.85}"
}
}
}