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

Plugin registry implementation #115

Merged
merged 7 commits into from
Jun 4, 2020
Merged

Plugin registry implementation #115

merged 7 commits into from
Jun 4, 2020

Conversation

graeme-a-stewart
Copy link
Member

This is not quite finished in it's full implementation, but it is a good time for review as further development comes on top.

Inspired by https://github.com/psalvaggio/cppregpattern this is a registry implementaion for prmon.

It makes the code in the main monitor look a lot nicer, as initialisation is now a loop over the registry. I am not completely happy with the pointer grabbed to wallmon, but as it's used inside the monitoring loop (to pass wallclock time to the average stats generation) it is needed and I could not think of a better way.

Closes #113
Helps with making #106 and #107 much easier to implement

@graeme-a-stewart graeme-a-stewart added the enhancement New feature or request label Jun 2, 2020
@amete amete added the work in progress Further development needed before this pull request can be accepted label Jun 3, 2020
@graeme-a-stewart graeme-a-stewart removed the work in progress Further development needed before this pull request can be accepted label Jun 3, 2020
@graeme-a-stewart
Copy link
Member Author

Hi @amete - so I think this in good shape to me merged now, if you approve of the changes

Proof-of-concept with one component using the
https://github.com/psalvaggio/cppregpattern
implementation
Get rid of some of the generic macros that we do not need
Add a description map that allows components to describe themselves
In our case the class name can always be used
Each monitor registers itself in its header
Main program loops over registered monitors to initialise
Much less hacky for getting the wallmon pointer
that is needed in the main loop

Cleanup some unused headers
@amete
Copy link
Collaborator

amete commented Jun 4, 2020

Hi @graeme-a-stewart, Do we really want to return a raw pointer w/ registry::Registry<Imonitor>::create? In the main application, we feed this into a std::unique_ptr so I don't think there is a problem per se, but I thought it would be nicer if the factory dealt w/ std::unique_ptr instead of raw pointers. If you're happy w/ this, I'll go ahead and merge.

@graeme-a-stewart
Copy link
Member Author

That's a good point! Yes, let me try to make that change then.

In prmon we don't have need for returning anything else, so
safer to embed this deeper into the code
package/src/registry.h Show resolved Hide resolved
package/src/registry.h Outdated Show resolved Hide resolved
package/src/prmon.cpp Show resolved Hide resolved
@amete
Copy link
Collaborator

amete commented Jun 4, 2020

All looks good to me, merging now.

@amete amete merged commit 005949b into master Jun 4, 2020
@graeme-a-stewart graeme-a-stewart deleted the plugin-registry branch June 4, 2020 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add registry for monitoring components
2 participants