Sensor optimization

SensorsAsyncResp has existed for a long time, and has slowly morphed
from its intended usage (as an RAII response object) into a
conglomeration of all possible things that a sensor request could want.
This leads to a ton of inefficient queries, and lots of data being held
for much longer than we'd like.

This commit tries to start breaking things apart, and follow the
patterns we use elsewhere, passing AsyncResp where a response object is
needed, and passing specialized data structures only into the scopes
where they're needed.  This significantly increases the performance of
the /redfish/v1/Chassis/<>/Sensors/<sensor> URI.

The optimization changes the URI such that in includes both the sensor
type as well as the sensor name in the URI, meaning that from a given
tree, we can directly look up the sensor path, instead of having to look
up all sensor paths, and do a filename() compare on them.

Implementation-wise, there is one main difference in user-facing
behavior, in that instead of using a mechanized version of the sensor
name for the URI (aka /redfish/v1/Chassis/my_chassis/Sensors/my_sensor)
the URI now contains the sensor type (ex
/redfish/v1/Chassis/my_chassis/Sensors/temperature_my_sensor).  One
implementation note: because fan_pwm and fan_tach namespaces have an
underscore in them, we normalize these in the URI to fanpwm and fantach
respectively such that we can differentiate between the two without
looping, and special case them on the other side.  This seems like a
reasonable compromise.

The above means that when a request comes in to query the sensor, we no
longer have to pull all sensors to identify the one that matches the
name, and we can go directly to the mapper to determine which sensor we
need, with a GetObject query.  This significantly reduces the amount of
time to grab the information from a single sensor.

To accomplish this, the per-sensor methods needed broken down into
pieces that allowed loading a single sensor at a time, rather than a
complete GetManagedObjects call.  In practice, this just means breaking
out one helper function, such that the new code can directly call
GetAll.

In a few places, const std::string& had to be replaced with
std::string_view, because the new sensor API can directly inline its
const char* parameters for types, which allows it to avoid constructing
a string copy to do it.

Tested:
Redfish service validator passes on a S7106 system, and shows a timing
of ~40-50ms per sensor request, which is in line with what we'd expect
for a keepalive function using Session auth.

'''
curl --insecure -w "@curl-format.txt" -H "X-Auth-Token: nOIarWLRFkFN14qVONs0" https://192.168.10.140/redfish/v1/Chassis/Tyan_S7106_Baseboard/Sensors/temperature_sys_air_inlet
'''

returns timing that is on the order of 125ms.  On this setup,
ServiceRoot (which should do no dbus calls) returns in 90ms, so the
sensor implementation itself is on the order of 40% of the timing.

TelemetryService functions as expected
'''
curl -k --user "root:0penBmc" -X POST https://$bmc/redfish/v1/TelemetryService/MetricReportDefinitions/ -d '{"Id": "lxw1", "Metrics": [{"MetricId": "123", "MetricProperties": ["/redfish/v1/Chassis/Tyan_S7106_Baseboard/Power#/Voltages/0"]}], "MetricReportDefinitionType": "OnRequest", "ReportActions": ["LogToMetricReportsCollection"], "Schedule": {"RecurrenceInterval": "100"}}'
'''

Succeeds.

Also succeeds with MetricProperties set to:
/redfish/v1/Chassis/Tyan_S7106_Baseboard/Sensors/voltage_vcc5

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: If42f531b385c3b611b100c1c485a1e4e877c5512

2 files changed, 111 insertions, 89 deletions

diff --git a/redfish-core/include/utils/telemetry_utils.hpp b/redfish-core/include/utils/telemetry_utils.hpp
index 49e1e12240..71e744983f 100644
--- a/redfish-core/include/utils/telemetry_utils.hpp
+++ b/redfish-core/include/utils/telemetry_utils.hpp
@@ -63,12 +63,10 @@ inline std::optional<IncorrectMetricUri> getChassisSensorNode(
         // Those 2 segments cannot be validated here, as we don't know which
         // sensors exist at the moment of parsing.
         std::string ignoredSenorId;
-        std::string ignoredSensorUnit;
 
         if (crow::utility::readUrlSegments(*parsed, "redfish", "v1", "Chassis",
                                            std::ref(chassis), "Sensors",
-                                           std::ref(ignoredSenorId),
-                                           std::ref(ignoredSensorUnit)))
+                                           std::ref(ignoredSenorId)))
         {
             matched.emplace(std::move(chassis), "Sensors");
             uriIdx++;
diff --git a/redfish-core/lib/sensors.hpp b/redfish-core/lib/sensors.hpp
index 7d09ce1904..d0485f0136 100644
--- a/redfish-core/lib/sensors.hpp
+++ b/redfish-core/lib/sensors.hpp
@@ -192,7 +192,6 @@ class SensorsAsyncResp
     {
         const std::string name;
         std::string uri;
-        const std::string valueKey;
         const std::string dbusPath;
     };
 
@@ -245,8 +244,7 @@ class SensorsAsyncResp
             {
                 for (auto& sensor : *metadata)
                 {
-                    map.insert(std::make_pair(sensor.uri + sensor.valueKey,
-                                              sensor.dbusPath));
+                    map.emplace(sensor.uri, sensor.dbusPath);
                 }
             }
             dataComplete(asyncResp->res.result(), map);
@@ -259,13 +257,12 @@ class SensorsAsyncResp
     SensorsAsyncResp& operator=(SensorsAsyncResp&&) = delete;
 
     void addMetadata(const nlohmann::json& sensorObject,
-                     const std::string& valueKey, const std::string& dbusPath)
+                     const std::string& dbusPath)
     {
         if (metadata)
         {
-            metadata->emplace_back(SensorData{sensorObject["Name"],
-                                              sensorObject["@odata.id"],
-                                              valueKey, dbusPath});
+            metadata->emplace_back(SensorData{
+                sensorObject["Name"], sensorObject["@odata.id"], dbusPath});
         }
     }
 
@@ -823,7 +820,17 @@ inline void objectPropertiesToJson(
 
     if (chassisSubNode == sensors::node::sensors)
     {
-        sensorJson["Id"] = sensorName;
+        std::string subNodeEscaped(chassisSubNode);
+        subNodeEscaped.erase(
+            std::remove(subNodeEscaped.begin(), subNodeEscaped.end(), '_'),
+            subNodeEscaped.end());
+
+        // For sensors in SensorCollection we set Id instead of MemberId,
+        // including power sensors.
+        subNodeEscaped += '_';
+        subNodeEscaped += sensorName;
+        sensorJson["Id"] = std::move(subNodeEscaped);
+
         std::string sensorNameEs(sensorName);
         std::replace(sensorNameEs.begin(), sensorNameEs.end(), '_', ' ');
         sensorJson["Name"] = std::move(sensorNameEs);
@@ -1040,8 +1047,6 @@ inline void objectPropertiesToJson(
             }
         }
     }
-
-    BMCWEB_LOG_DEBUG << "Added sensor " << sensorName;
 }
 
 /**
@@ -1068,6 +1073,7 @@ inline void objectInterfacesToJson(
         objectPropertiesToJson(sensorName, sensorType, chassisSubNode,
                                valuesDict, sensorJson, inventoryItem);
     }
+    BMCWEB_LOG_DEBUG << "Added sensor " << sensorName;
 }
 
 inline void populateFanRedundancy(
@@ -2453,10 +2459,20 @@ inline void getSensorData(
                 if (sensorSchema == sensors::node::sensors &&
                     !sensorsAsyncResp->efficientExpand)
                 {
+                    std::string sensorTypeEscaped(sensorType);
+                    sensorTypeEscaped.erase(
+                        std::remove(sensorTypeEscaped.begin(),
+                                    sensorTypeEscaped.end(), '_'),
+                        sensorTypeEscaped.end());
+                    std::string sensorId(sensorTypeEscaped);
+                    sensorId += "_";
+                    sensorId += sensorName;
+
                     sensorsAsyncResp->asyncResp->res.jsonValue["@odata.id"] =
-                        "/redfish/v1/Chassis/" + sensorsAsyncResp->chassisId +
-                        "/" + sensorsAsyncResp->chassisSubNode + "/" +
-                        sensorName;
+                        crow::utility::urlFromPieces(
+                            "redfish", "v1", "Chassis",
+                            sensorsAsyncResp->chassisId,
+                            sensorsAsyncResp->chassisSubNode, sensorId);
                     sensorJson = &(sensorsAsyncResp->asyncResp->res.jsonValue);
                 }
                 else
@@ -2565,8 +2581,7 @@ inline void getSensorData(
                     path += sensorType;
                     path += "/";
                     path += sensorName;
-                    sensorsAsyncResp->addMetadata(*sensorJson, sensorType,
-                                                  path);
+                    sensorsAsyncResp->addMetadata(*sensorJson, path);
                 }
             }
             if (sensorsAsyncResp.use_count() == 1)
@@ -2887,14 +2902,14 @@ namespace sensors
 {
 
 inline void getChassisCallback(
-    const std::shared_ptr<SensorsAsyncResp>& asyncResp,
+    const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+    std::string_view chassisId, std::string_view chassisSubNode,
     const std::shared_ptr<std::set<std::string>>& sensorNames)
 {
-    BMCWEB_LOG_DEBUG << "getChassisCallback enter";
+    BMCWEB_LOG_DEBUG << "getChassisCallback enter ";
 
-    nlohmann::json& entriesArray =
-        asyncResp->asyncResp->res.jsonValue["Members"];
-    for (const auto& sensor : *sensorNames)
+    nlohmann::json& entriesArray = asyncResp->res.jsonValue["Members"];
+    for (const std::string& sensor : *sensorNames)
     {
         BMCWEB_LOG_DEBUG << "Adding sensor: " << sensor;
 
@@ -2903,18 +2918,25 @@ inline void getChassisCallback(
         if (sensorName.empty())
         {
             BMCWEB_LOG_ERROR << "Invalid sensor path: " << sensor;
-            messages::internalError(asyncResp->asyncResp->res);
+            messages::internalError(asyncResp->res);
             return;
         }
+        std::string type = path.parent_path().filename();
+        // fan_tach has an underscore in it, so remove it to "normalize" the
+        // type in the URI
+        type.erase(std::remove(type.begin(), type.end(), '_'), type.end());
+
         nlohmann::json::object_t member;
-        member["@odata.id"] = "/redfish/v1/Chassis/" + asyncResp->chassisId +
-                              "/" + asyncResp->chassisSubNode + "/" +
-                              sensorName;
+        std::string id = type;
+        id += "_";
+        id += sensorName;
+        member["@odata.id"] = crow::utility::urlFromPieces(
+            "redfish", "v1", "Chassis", chassisId, chassisSubNode, id);
+
         entriesArray.push_back(std::move(member));
     }
 
-    asyncResp->asyncResp->res.jsonValue["Members@odata.count"] =
-        entriesArray.size();
+    asyncResp->res.jsonValue["Members@odata.count"] = entriesArray.size();
     BMCWEB_LOG_DEBUG << "getChassisCallback exit";
 }
 
@@ -2947,95 +2969,97 @@ inline void
         return;
     }
 
-    // if there's no efficient expand available, we use the default
-    // Query Parameters route
-    auto asyncResp = std::make_shared<SensorsAsyncResp>(
-        aResp, chassisId, sensors::dbus::sensorPaths, sensors::node::sensors);
-
     // We get all sensors as hyperlinkes in the chassis (this
     // implies we reply on the default query parameters handler)
-    getChassis(asyncResp->asyncResp, asyncResp->chassisId,
-               asyncResp->chassisSubNode, asyncResp->types,
+    getChassis(aResp, chassisId, sensors::node::sensors, dbus::sensorPaths,
+               std::bind_front(sensors::getChassisCallback, aResp, chassisId,
+                               sensors::node::sensors));
+}
 
-               std::bind_front(sensors::getChassisCallback, asyncResp));
-    BMCWEB_LOG_DEBUG << "SensorCollection doGet exit";
+inline void
+    getSensorFromDbus(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+                      const std::string& sensorPath,
+                      const ::dbus::utility::MapperGetObject& mapperResponse)
+{
+    if (mapperResponse.size() != 1)
+    {
+        messages::internalError(asyncResp->res);
+        return;
+    }
+    const auto& valueIface = *mapperResponse.begin();
+    const std::string& connectionName = valueIface.first;
+    BMCWEB_LOG_DEBUG << "Looking up " << connectionName;
+    BMCWEB_LOG_DEBUG << "Path " << sensorPath;
+    crow::connections::systemBus->async_method_call(
+        [asyncResp,
+         sensorPath](const boost::system::error_code ec,
+                     const ::dbus::utility::DBusPropertiesMap& valuesDict) {
+        if (ec)
+        {
+            messages::internalError(asyncResp->res);
+            return;
+        }
+        sdbusplus::message::object_path path(sensorPath);
+        std::string name = path.filename();
+        path = path.parent_path();
+        std::string type = path.filename();
+        objectPropertiesToJson(name, type, sensors::node::sensors, valuesDict,
+                               asyncResp->res.jsonValue, nullptr);
+        },
+        connectionName, sensorPath, "org.freedesktop.DBus.Properties", "GetAll",
+        "");
 }
 
 inline void handleSensorGet(App& app, const crow::Request& req,
-                            const std::shared_ptr<bmcweb::AsyncResp>& aResp,
-                            const std::string& chassisId,
-                            const std::string& sensorName)
+                            const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+                            const std::string& /*chassisId*/,
+                            const std::string& sensorId)
 {
-    if (!redfish::setUpRedfishRoute(app, req, aResp))
+    if (!redfish::setUpRedfishRoute(app, req, asyncResp))
+    {
+        return;
+    }
+    size_t index = sensorId.find('_');
+    if (index == std::string::npos)
     {
+        messages::resourceNotFound(asyncResp->res, sensorId, "Sensor");
         return;
     }
+    std::string sensorType = sensorId.substr(0, index);
+    std::string sensorName = sensorId.substr(index + 1);
+    // fan_pwm and fan_tach need special handling
+    if (sensorType == "fantach" || sensorType == "fanpwm")
+    {
+        sensorType.insert(3, 1, '_');
+    }
+
     BMCWEB_LOG_DEBUG << "Sensor doGet enter";
-    std::shared_ptr<SensorsAsyncResp> asyncResp =
-        std::make_shared<SensorsAsyncResp>(aResp, chassisId,
-                                           std::span<std::string_view>(),
-                                           sensors::node::sensors);
 
     const std::array<const char*, 1> interfaces = {
         "xyz.openbmc_project.Sensor.Value"};
-
+    std::string sensorPath =
+        "/xyz/openbmc_project/sensors/" + sensorType + '/' + sensorName;
     // Get a list of all of the sensors that implement Sensor.Value
     // and get the path and service name associated with the sensor
     crow::connections::systemBus->async_method_call(
-        [asyncResp,
+        [asyncResp, sensorPath,
          sensorName](const boost::system::error_code ec,
-                     const ::dbus::utility::MapperGetSubTreeResponse& subtree) {
+                     const ::dbus::utility::MapperGetObject& subtree) {
         BMCWEB_LOG_DEBUG << "respHandler1 enter";
         if (ec)
         {
-            messages::internalError(asyncResp->asyncResp->res);
+            messages::internalError(asyncResp->res);
             BMCWEB_LOG_ERROR << "Sensor getSensorPaths resp_handler: "
                              << "Dbus error " << ec;
             return;
         }
-
-        ::dbus::utility::MapperGetSubTreeResponse::const_iterator it =
-            std::find_if(
-                subtree.begin(), subtree.end(),
-                [sensorName](
-                    const std::pair<
-                        std::string,
-                        std::vector<std::pair<
-                            std::string, std::vector<std::string>>>>& object) {
-            sdbusplus::message::object_path path(object.first);
-            std::string name = path.filename();
-            if (name.empty())
-            {
-                BMCWEB_LOG_ERROR << "Invalid sensor path: " << object.first;
-                return false;
-            }
-
-            return name == sensorName;
-                });
-
-        if (it == subtree.end())
-        {
-            BMCWEB_LOG_ERROR << "Could not find path for sensor: "
-                             << sensorName;
-            messages::resourceNotFound(asyncResp->asyncResp->res, "Sensor",
-                                       sensorName);
-            return;
-        }
-        std::string_view sensorPath = (*it).first;
-        BMCWEB_LOG_DEBUG << "Found sensor path for sensor '" << sensorName
-                         << "': " << sensorPath;
-
-        const std::shared_ptr<std::set<std::string>> sensorList =
-            std::make_shared<std::set<std::string>>();
-
-        sensorList->emplace(sensorPath);
-        processSensorList(asyncResp, sensorList);
+        getSensorFromDbus(asyncResp, sensorPath, subtree);
         BMCWEB_LOG_DEBUG << "respHandler1 exit";
         },
         "xyz.openbmc_project.ObjectMapper",
         "/xyz/openbmc_project/object_mapper",
-        "xyz.openbmc_project.ObjectMapper", "GetSubTree",
-        "/xyz/openbmc_project/sensors", 2, interfaces);
+        "xyz.openbmc_project.ObjectMapper", "GetObject", sensorPath,
+        interfaces);
 }
 
 } // namespace sensors