[SERVER-65987] ServerStatusMetric API refresh Created: 26/Apr/22 Updated: 29/Oct/23 Resolved: 12/Jul/22 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 6.1.0-rc0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Billy Donahue | Assignee: | Billy Donahue |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||||||||||
| Operating System: | ALL | ||||||||||||||||||||||||||||
| Sprint: | Service Arch 2022-05-30, Service Arch 2022-06-13, Service Arch 2022-06-27, Service Arch 2022-07-11, Service Arch 2022-07-25 | ||||||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||||||
| Description |
|
The ServerStatusMetric class manipulates the MetricTree singleton directly, passing an incomplete this pointer from the base class constructor. This is a violation of separation of concern. The singleton's state is not ServerStatusMetric's job. We have better registry/registration patterns we could use here, including MONGO_INITIALIZER or a more specialized registration API. Creation of new ServerStatusMetric instances should be done with a factory that creates them and then registers them only when the instance is fully created. Registering before that is to pass an incomplete object, which is an unnecessary risk. What happens now:
This is a quick cleanup that will make this subsystem cleaner and safer. |
| Comments |
| Comment by Githook User [ 30/Jun/22 ] | ||||||||||
|
Author: {'name': 'Daniel Morilha', 'email': 'daniel.morilha@mongodb.com', 'username': 'daniel-mdb'}Message: | ||||||||||
| Comment by Billy Donahue [ 18/May/22 ] | ||||||||||
|
The std::make_shared, etc convention is bad in a few ways though. It requires the maker function to be able to name the T type and to have access to the necessary constructor. It also forbids brace-init or nested brace init for the Args. The Args all have to have types, which can be a pain. It's easier if a client can make a unique_ptr<ServerStatusMetric> through completely private means and add it to any MetricTree they want to add it to. If that's "theMetricTree" great. But for testing I want to pick one (or none) myself This is an improvement over what we currently have to be sure, but I think the create function is doing a lot here. It is managing creating the singleton, and then constructing a T and then adding the T to the singleton. I would prefer to see these three separate actions as available primitives, and then a function like create as a higher-layer convenience helper if it makes sense to define it. It would not be a member of MetricTree necessarily, just a thing that uses the public API of metric tree in a way that helps conformance to a conventional init sequence for ServerStatusMetric objects. | ||||||||||
| Comment by Daniel Morilha (Inactive) [ 18/May/22 ] | ||||||||||
|
A possible solution I've already explored is to instead create the objects (similarly to what std::make convention does) from MetricTree itself, here's what I tried:
What I particularly like about this approach is that the lifetime of all associated ServerStatusMetric derived instances are now tied to MetricTree (for some weird reason I keep calling it MagicTree) rather than their creator which may have questionable lifespan ownership. |