-
Notifications
You must be signed in to change notification settings - Fork 113
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
Read metrics from file #973
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
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.
@redblom this implementation puts a lot of restrictions on the prometheus HTTP service, plus there are a lot of if/else conditions in the metrics package.
- Any service or package should be able to register their views to prometheus to which prometheus should be agnostic. Maybe in the future, the storageprovider service wants to add a few metrics- adding those will involve even more changes to this service which is quite inefficient.
- The different drivers need to operate in their own ways. The dummy driver might want to run in the background, the json implementation needs to read the metrics file every time it is called (which in this implementation, it doesn't, it only reads it at the beginning). So adding the rigid constraints in the metrics.go file is not ideal. That should simply define the methods each of which need to implement, and they can use their own data stores and structs.
- The config for each driver is different, so using a common struct is not ideal, plus we shouldn't link it to the prometheus service.
So please go through #934. You can rename the script driver to json, then add the logic for the methods. Also, you'll need to read the json file every time a call to read the metrics is made. Also, instead of calling the init method here, we can add a loader package inside the metrics package where we can import all the drivers whose metrics we want to register.
There's one problem which comes up in this approach, which is how to specify the location of the json file. That can be done through sharedconf. It can be configured like this and used like this.
Also, instead of the current code, where we sleep between reading the metrics, let's use the stats channel as done here. |
@ishank011 @labkode : I see some issues here. First, to our understanding there can only be one single source from which the data for a particular metric can come from (metrics methods are defined in the Reader interface). Meaning, although there may be multiple implementations (ie. drivers) for the metrics interface there can only be one used at runtime (again: there can only be one source). So, we cannot have 'dummy' implementation running in the background as that would mean a second data source for the same metrics. So, that's why I had multiple (json, dummy) drivers (data sources) coded, implementing the Reader interface, so choosing the one you'd like to use would be a matter of configuration. We also envision that a site could choose its own data source, so possibly another driver (eg. db) that has to implement the Reader interface. |
@ishank011 @labkode : This drawing is what we understand of 'read metrics from file'. It might help to pinpoint where our views on this may diverge. |
@ishank011 : created new PR #1118 |
#698
In the metrics.toml config, specify which metrics driver to use (key: metrics_data_driver_type), one of: dummy, json
By default dummy is set, giving some ficticious metrics values.
For json a default json metrics data file location (/var/tmp/reva/metrics/metricsdata.json) is used. Otherwise set the metrics_data_location key in the metrics.toml file.
The Reader interface method signatures have changed to return the metrics values. Any driver must implement this interface.