[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:
Duplicate
is duplicated by SERVER-67333 ServerStatusMetric API refresh follow-up Closed
Related
related to SERVER-67764 Coverity analysis defect 122495: Reso... Closed
related to SERVER-67647 Potentially optimize mongo::MetricTre... Open
related to SERVER-65984 Move ServerStatusMetricField speciali... Closed
related to SERVER-67644 Certain ServerStatusMetricFields can ... Closed
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:

ServerStatusMetric::ServerStatusMetric(const string& nameIn)
    : _name(nameIn), _leafName(_parseLeafName(nameIn)) {
    if (MetricTree::theMetricTree == nullptr)
        MetricTree::theMetricTree = new MetricTree();
    MetricTree::theMetricTree->add(this);
}

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: SERVER-65987 ServerStatusMetric API refresh
Branch: master
https://github.com/mongodb/mongo/commit/fae8410364c1f02ee62e77076ef60b8ee418368f

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:

 class MetricTree {
    template <typename T, typename... Args>
    static T& create(Args&&... args) {
        if (MetricTree::theMetricTree == nullptr)
            MetricTree::theMetricTree = new MetricTree();
        auto metric = std::unique_ptr<T>(new T(std::forward<Args>(args)...));
        T& result = *metric;
        MetricTree::theMetricTree->add(std::move(metric));
        return result;
    }

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.

Generated at Thu Feb 08 06:04:10 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.