Skip to content
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

Closed
wants to merge 16 commits into from
Closed

Conversation

redblom
Copy link
Contributor

@redblom redblom commented Jul 14, 2020

#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.

@redblom redblom requested a review from labkode as a code owner July 14, 2020 09:19
@update-docs
Copy link

update-docs bot commented Jul 14, 2020

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.

@hound hound bot deleted a comment from redblom Jul 14, 2020
Copy link
Contributor

@ishank011 ishank011 left a 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.

@ishank011
Copy link
Contributor

Also, instead of the current code, where we sleep between reading the metrics, let's use the stats channel as done here.

@redblom
Copy link
Contributor Author

redblom commented Jul 20, 2020

@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.
So, if in the future eg. storage driver (as you mentioned) would want to have some additional (other!!) metrics registered then we would expand the Reader interface with these additional metrics methods, and these methods would then need to be implemented by all metrics data drivers so the new metrics would also be read from the data source.
Now in this view prometheus only needs to initialize the Metrics construct + configure which driver to choose and you're good to go (it does work actually!). The metrics.RecordMetrics() function (as used in prometheus.go line 82) is there because I thought we should get rid of the continuous running record job (I just thought that it would be only necessary to read data when the prometheus server is called), but I understand that is what you want. No problem!
I have an issue with using the Reader interface like you do. Checking whether the interface is actually implemented is done within the implementing driver itself, that's not really checking, it’s you checking you. This makes the interface sort of useless. That's why I made sure that in the metrics pkg the driver is of type Reader (that way if a method is not implemented you get a nice message when building, meaning the driver is not complete). If Reader interface is not used like this it hardly gives any advantage and we can get rid of the it completely and make the drivers define all metrics methods themselves. Less code.
I must confess I'm not experienced in golang so my implementation/coding of these views may be a bit clumsy here and there. All comments on that is super helpful to me !
So, here are our views on the mechanics behind my PR. I want to make clear that I have no problem implementing it the way you've suggested. We were under the impression that having the metrics read from a data source (file,db) would be a short time solution anyway (hopefully metrics will become a part of the CS3 API's in the near future). I just want to make sure we understand each other. Hey, we can always do a videocall on this.
So please skip my comments and tell me to proceed with your suggestion if you like. No problemo!

@redblom
Copy link
Contributor Author

redblom commented Jul 20, 2020

@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.

metrics_pkg_module

@redblom
Copy link
Contributor Author

redblom commented Aug 25, 2020

@ishank011 : created new PR #1118

@ishank011 ishank011 closed this Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants