[FEATURE REQ] provide functionality for user defined refresh rates about grofer HOT 12 CLOSED

@yongruilin, for now, let's keep the default refresh rate to be 1000 ms and the refresh rate should be provided in ms by the user. As @Gituser143 mentioned above, for the purposes of rendering and refreshing the UI, anything less than 1000 ms can get very heavy on the CPU and not look great as you mentioned. So instead of creating a PersistentFlag for refresh rate on the root command in cmd/root.go, maybe you could create non-persistent flags in cmd/root.go and cmd/proc.go and have a check for the refresh rate provided to be at a minimum of 1000 ms and if not you could return an error suggesting the same!

For example:
In cmd/root.go you could add in the function init() the following:

rootCmd.Flags().Int32P("refresh", "r", 1000, "UI refresh rate in milliseconds")

and something similar in cmd/proc.go as well!

We plan to add features for exporting data sometime in the near future, so for those commands, a refresh rate of less than 1000 ms will work!

from grofer.

Setting the priority of the issue to be high since the further set of features will depend on user-defined configurations

from grofer.

Hi @MadhavJivrajani , I found this repo very interesting. Can I have a try on this feature? Should the default refresh rate all the same for OverallGraph and procGraph? It doesn't look good on the overallGraph with the refresh rate on 100ms.

from grofer.

@yongruilin let's keep the default refresh rates of the UI at 1000 milli seconds. Allow the user to give custom input in terms of seconds?

from grofer.

Hey @yongruilin !
Yes, sure! I'll assign you to this issue, also could you elaborate on what exactly doesn't look good or is messing up?

from grofer.

@Gituser143 let the rate be provided in terms of ms itself. It'll give more fine-grained control in terms of refresh rates especially when we move to add features about exporting data

So maybe if 100 ms isn't working out, let the default be 1000 and then the user can adjust it as required.

from grofer.

@MadhavJivrajani I suggest we keep the minimum refresh rate of the UI as 1000 ms, while exporting stats, the UI refresh rate won't really matter to us right? In those cases we can lower it if needed.

from grofer.

The refresh rate provided by the user, the same can be used to see how frequently data should be fetched and exported, sort of analogous to how frequently data will refresh on the UI

from grofer.

I agree with fetching and exporting of data, but keeping UI refresh rates lesser than 1000 ms would cause some glitches for the UI. EDIT: It also has additional CPU overheads as it consumes a significantly higher amount if refreshed in intervals lesser than 1000 ms

from grofer.

@MadhavJivrajani
I don't think it is appropriate to have a non-persistent flag for refresh rate as it should be available everywhere https://github.com/spf13/cobra/blob/master/README.md#persistent-flags

Other wise it would panic with the same flag name in both root and children command.

panic: grofer flag redefined: refresh

from grofer.

• If I understand correctly, in the cases of nonpersistent flags, cobra processes flags that are local only to the target command: https://github.com/spf13/cobra/blob/master/README.md#local-flags
• The error that you mentioned, I tried assigning non-persistent flags to the two commands without doing anything with their values, it seems to be working fine for me. Could you walk me through the steps of reproducing it?
• Yes, ideally we would want refresh rate to be same everywhere. But here we want it to be same for grofer and grofer proc [-p PID]. In the future, there will be more commands such as grofer profile which will export profiled data, so for that command, we don't need to have a restriction on the minimum refresh rate as the UI wont be in picture there

from grofer.

Yes, that works for me as well.

The restriction is set in the function of RunE. We can define different restriction for different subcommand. If in the future we have grofer profile we can also set different restriction. We can just be more specific in the help and error output.

Still think that if the flag name is the same from both root and subcommand, they should be put in the same place.
Actually I suggest not to have any feature function from the root command. How about we put the overGraph to another subcommand like grofer overall?

from grofer.