Improved telemetry service error handling
By using data that is send in error massage it is possible to report
detailed information about error to user. Now instead of performing same
validation in bmcweb and backend it can be performed only in backend and
send back detailed error information.
Tested:
- Tested with https://gerrit.openbmc.org/c/openbmc/telemetry/+/57177
- All telemetry features are working as before
- Error codes in bmcweb are more detailed, for example invalid argument
instead of internal error.
Change-Id: I424d91b9c8d70c8320f66b582e110c2e7b906107
Signed-off-by: Krzysztof Grobelny <krzysztof.grobelny@intel.com>
Signed-off-by: Eryk Siejk <erykx.siejk@intel.com>
Signed-off-by: Michal Orzel <michalx.orzel@intel.com>
diff --git a/redfish-core/lib/metric_report_definition.hpp b/redfish-core/lib/metric_report_definition.hpp
index eca2743..d92343d 100644
--- a/redfish-core/lib/metric_report_definition.hpp
+++ b/redfish-core/lib/metric_report_definition.hpp
@@ -650,6 +650,105 @@
return true;
}
+inline std::string toRedfishProperty(std::string_view dbusMessage)
+{
+ if (dbusMessage == "Id")
+ {
+ return "Id";
+ }
+ if (dbusMessage == "Name")
+ {
+ return "Name";
+ }
+ if (dbusMessage == "ReportingType")
+ {
+ return "MetricReportDefinitionType";
+ }
+ if (dbusMessage == "AppendLimit")
+ {
+ return "AppendLimit";
+ }
+ if (dbusMessage == "ReportActions")
+ {
+ return "ReportActions";
+ }
+ if (dbusMessage == "Interval")
+ {
+ return "RecurrenceInterval";
+ }
+ if (dbusMessage == "ReportUpdates")
+ {
+ return "ReportUpdates";
+ }
+ if (dbusMessage == "ReadingParameters")
+ {
+ return "Metrics";
+ }
+ return "";
+}
+
+inline bool handleParamError(crow::Response& res, const char* errorMessage,
+ std::string_view key)
+{
+ if (errorMessage == nullptr)
+ {
+ BMCWEB_LOG_ERROR("errorMessage was null");
+ return true;
+ }
+ std::string_view errorMessageSv(errorMessage);
+ if (errorMessageSv.starts_with(key))
+ {
+ std::string redfishProperty = toRedfishProperty(key);
+ if (redfishProperty.empty())
+ {
+ // Getting here means most possibly that toRedfishProperty has
+ // incomplete implementation. Return internal error for now.
+ BMCWEB_LOG_ERROR("{} has no corresponding Redfish property", key);
+ messages::internalError(res);
+ return false;
+ }
+ messages::propertyValueError(res, redfishProperty);
+ return false;
+ }
+
+ return true;
+}
+
+inline void afterAddReport(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+ const AddReportArgs& args,
+ const boost::system::error_code& ec,
+ const sdbusplus::message_t& msg)
+{
+ if (!ec)
+ {
+ messages::created(asyncResp->res);
+ return;
+ }
+
+ if (ec == boost::system::errc::invalid_argument)
+ {
+ const sd_bus_error* errorMessage = msg.get_error();
+ if (errorMessage != nullptr)
+ {
+ for (const auto& arg :
+ {"Id", "Name", "ReportingType", "AppendLimit", "ReportActions",
+ "Interval", "ReportUpdates", "ReadingParameters"})
+ {
+ if (!handleParamError(asyncResp->res, errorMessage->message,
+ arg))
+ {
+ return;
+ }
+ }
+ }
+ }
+ if (!verifyCommonErrors(asyncResp->res, args.id, ec))
+ {
+ return;
+ }
+ messages::internalError(asyncResp->res);
+}
+
class AddReport
{
public:
@@ -708,40 +807,11 @@
std::move(sensorParams), metric.collectionFunction,
metric.collectionTimeScope, metric.collectionDuration);
}
-
crow::connections::systemBus->async_method_call(
- [asyncResp, id = args.id, uriToDbus](
- const boost::system::error_code& ec, const std::string&) {
- if (ec == boost::system::errc::file_exists)
- {
- messages::resourceAlreadyExists(
- asyncResp->res, "MetricReportDefinition", "Id", id);
- return;
- }
- if (ec == boost::system::errc::too_many_files_open)
- {
- messages::createLimitReachedForResource(asyncResp->res);
- return;
- }
- if (ec == boost::system::errc::argument_list_too_long)
- {
- nlohmann::json metricProperties = nlohmann::json::array();
- for (const auto& [uri, _] : uriToDbus)
- {
- metricProperties.emplace_back(uri);
- }
- messages::propertyValueIncorrect(
- asyncResp->res, "MetricProperties", metricProperties);
- return;
- }
- if (ec)
- {
- messages::internalError(asyncResp->res);
- BMCWEB_LOG_ERROR("respHandler DBus error {}", ec);
- return;
- }
-
- messages::created(asyncResp->res);
+ [asyncResp, args](const boost::system::error_code& ec,
+ const sdbusplus::message_t& msg,
+ const std::string& /*arg1*/) {
+ afterAddReport(asyncResp, args, ec, msg);
},
telemetry::service, "/xyz/openbmc_project/Telemetry/Reports",
"xyz.openbmc_project.Telemetry.ReportManager", "AddReport",
@@ -761,11 +831,111 @@
}
private:
- std::shared_ptr<bmcweb::AsyncResp> asyncResp;
+ const std::shared_ptr<bmcweb::AsyncResp> asyncResp;
AddReportArgs args;
boost::container::flat_map<std::string, std::string> uriToDbus;
};
+inline std::optional<
+ std::vector<std::tuple<sdbusplus::message::object_path, std::string>>>
+ sensorPathToUri(
+ const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+ std::span<const std::string> uris,
+ const std::map<std::string, std::string>& metricPropertyToDbusPaths)
+{
+ std::vector<std::tuple<sdbusplus::message::object_path, std::string>>
+ result;
+
+ for (const std::string& uri : uris)
+ {
+ auto it = metricPropertyToDbusPaths.find(uri);
+ if (it == metricPropertyToDbusPaths.end())
+ {
+ messages::propertyValueNotInList(asyncResp->res, uri,
+ "MetricProperties");
+ return {};
+ }
+ result.emplace_back(it->second, uri);
+ }
+
+ return result;
+}
+
+inline void afterSetReadingParams(
+ const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+ const std::string& reportId, const boost::system::error_code& ec,
+ const sdbusplus::message_t& msg)
+{
+ if (!ec)
+ {
+ messages::success(asyncResp->res);
+ return;
+ }
+ if (ec == boost::system::errc::invalid_argument)
+ {
+ const sd_bus_error* errorMessage = msg.get_error();
+ if (errorMessage != nullptr)
+ {
+ for (const auto& arg : {"Id", "ReadingParameters"})
+ {
+ if (!handleParamError(asyncResp->res, errorMessage->message,
+ arg))
+ {
+ return;
+ }
+ }
+ }
+ }
+ if (!verifyCommonErrors(asyncResp->res, reportId, ec))
+ {
+ return;
+ }
+ messages::internalError(asyncResp->res);
+}
+
+inline void setReadingParams(
+ const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+ const std::string& reportId, ReadingParameters readingParams,
+ const std::vector<std::vector<std::string>>& readingParamsUris,
+ const std::map<std::string, std::string>& metricPropertyToDbusPaths)
+{
+ if (asyncResp->res.result() != boost::beast::http::status::ok)
+ {
+ return;
+ }
+
+ for (size_t index = 0; index < readingParamsUris.size(); ++index)
+ {
+ std::span<const std::string> newUris = readingParamsUris[index];
+
+ const std::optional<std::vector<
+ std::tuple<sdbusplus::message::object_path, std::string>>>
+ readingParam =
+ sensorPathToUri(asyncResp, newUris, metricPropertyToDbusPaths);
+
+ if (!readingParam)
+ {
+ return;
+ }
+
+ for (const std::tuple<sdbusplus::message::object_path, std::string>&
+ value : *readingParam)
+ {
+ std::get<0>(readingParams[index]).emplace_back(value);
+ }
+ }
+
+ crow::connections::systemBus->async_method_call(
+ [asyncResp, reportId](const boost::system::error_code& ec,
+ const sdbusplus::message_t& msg) {
+ afterSetReadingParams(asyncResp, reportId, ec, msg);
+ },
+ "xyz.openbmc_project.Telemetry", getDbusReportPath(reportId),
+ "org.freedesktop.DBus.Properties", "Set",
+ "xyz.openbmc_project.Telemetry.Report", "ReadingParameters",
+ dbus::utility::DbusVariantType{readingParams});
+}
+
class UpdateMetrics
{
public:
@@ -776,18 +946,11 @@
~UpdateMetrics()
{
- try
- {
- setReadingParams();
- }
- catch (const std::exception& e)
- {
- BMCWEB_LOG_ERROR("{}", e.what());
- }
- catch (...)
- {
- BMCWEB_LOG_ERROR("Unknown error");
- }
+ boost::asio::post(
+ crow::connections::systemBus->get_io_context(),
+ std::bind_front(&setReadingParams, asyncResp, id,
+ std::move(readingParams), readingParamsUris,
+ metricPropertyToDbusPaths));
}
UpdateMetrics(const UpdateMetrics&) = delete;
@@ -819,66 +982,7 @@
metricArgs.collectionDuration);
}
- void setReadingParams()
- {
- if (asyncResp->res.result() != boost::beast::http::status::ok)
- {
- return;
- }
-
- for (size_t index = 0; index < readingParamsUris.size(); ++index)
- {
- std::span<const std::string> newUris = readingParamsUris[index];
-
- const std::optional<std::vector<
- std::tuple<sdbusplus::message::object_path, std::string>>>
- readingParam = sensorPathToUri(newUris);
-
- if (!readingParam)
- {
- return;
- }
-
- std::get<0>(readingParams[index]) = *readingParam;
- }
-
- crow::connections::systemBus->async_method_call(
- [asyncResp(this->asyncResp),
- reportId = id](const boost::system::error_code& ec) {
- if (!verifyCommonErrors(asyncResp->res, reportId, ec))
- {
- return;
- }
- },
- "xyz.openbmc_project.Telemetry", getDbusReportPath(id),
- "org.freedesktop.DBus.Properties", "Set",
- "xyz.openbmc_project.Telemetry.Report", "ReadingParameters",
- dbus::utility::DbusVariantType{readingParams});
- }
-
private:
- std::optional<
- std::vector<std::tuple<sdbusplus::message::object_path, std::string>>>
- sensorPathToUri(std::span<const std::string> uris) const
- {
- std::vector<std::tuple<sdbusplus::message::object_path, std::string>>
- result;
-
- for (const std::string& uri : uris)
- {
- auto it = metricPropertyToDbusPaths.find(uri);
- if (it == metricPropertyToDbusPaths.end())
- {
- messages::propertyValueNotInList(asyncResp->res, uri,
- "MetricProperties");
- return {};
- }
- result.emplace_back(it->second, uri);
- }
-
- return result;
- }
-
const std::shared_ptr<bmcweb::AsyncResp> asyncResp;
std::vector<std::vector<std::string>> readingParamsUris;
ReadingParameters readingParams;
@@ -901,6 +1005,38 @@
dbus::utility::DbusVariantType{enabled});
}
+inline void afterSetReportingProperties(
+ const std::shared_ptr<bmcweb::AsyncResp>& asyncResp, const std::string& id,
+ const boost::system::error_code& ec, const sdbusplus::message_t& msg)
+{
+ if (!ec)
+ {
+ asyncResp->res.result(boost::beast::http::status::no_content);
+ return;
+ }
+
+ if (ec == boost::system::errc::invalid_argument)
+ {
+ const sd_bus_error* errorMessage = msg.get_error();
+ if (errorMessage != nullptr)
+ {
+ for (const auto& arg : {"Id", "ReportingType", "Interval"})
+ {
+ if (!handleParamError(asyncResp->res, errorMessage->message,
+ arg))
+ {
+ return;
+ }
+ }
+ }
+ }
+ if (!verifyCommonErrors(asyncResp->res, id, ec))
+ {
+ return;
+ }
+ messages::internalError(asyncResp->res);
+}
+
inline void setReportTypeAndInterval(
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp, std::string_view id,
const std::optional<std::string>& reportingType,
@@ -934,17 +1070,45 @@
}
crow::connections::systemBus->async_method_call(
- [asyncResp, id = std::string(id)](const boost::system::error_code& ec) {
- if (!verifyCommonErrors(asyncResp->res, id, ec))
- {
- return;
- }
+ [asyncResp, id = std::string(id)](const boost::system::error_code& ec,
+ const sdbusplus::message_t& msg) {
+ afterSetReportingProperties(asyncResp, id, ec, msg);
},
"xyz.openbmc_project.Telemetry", getDbusReportPath(id),
"xyz.openbmc_project.Telemetry.Report", "SetReportingProperties",
dbusReportingType, recurrenceInterval);
}
+inline void afterSetReportUpdates(
+ const std::shared_ptr<bmcweb::AsyncResp>& asyncResp, const std::string& id,
+ const boost::system::error_code& ec, const sdbusplus::message_t& msg)
+{
+ if (!ec)
+ {
+ asyncResp->res.result(boost::beast::http::status::no_content);
+ return;
+ }
+ if (ec == boost::system::errc::invalid_argument)
+ {
+ const sd_bus_error* errorMessage = msg.get_error();
+ if (errorMessage != nullptr)
+ {
+ for (const auto& arg : {"Id", "ReportUpdates"})
+ {
+ if (!handleParamError(asyncResp->res, errorMessage->message,
+ arg))
+ {
+ return;
+ }
+ }
+ }
+ }
+ if (!verifyCommonErrors(asyncResp->res, id, ec))
+ {
+ return;
+ }
+}
+
inline void
setReportUpdates(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
std::string_view id, const std::string& reportUpdates)
@@ -957,16 +1121,42 @@
return;
}
crow::connections::systemBus->async_method_call(
- [asyncResp, id = std::string(id)](const boost::system::error_code& ec) {
- if (!verifyCommonErrors(asyncResp->res, id, ec))
- {
- return;
- }
+ [asyncResp, id = std::string(id)](const boost::system::error_code& ec,
+ const sdbusplus::message_t& msg) {
+ afterSetReportUpdates(asyncResp, id, ec, msg);
},
"xyz.openbmc_project.Telemetry", getDbusReportPath(id),
"org.freedesktop.DBus.Properties", "Set",
"xyz.openbmc_project.Telemetry.Report", "ReportUpdates",
- dbus::utility::DbusVariantType{reportUpdates});
+ dbus::utility::DbusVariantType{dbusReportUpdates});
+}
+
+inline void afterSetReportActions(
+ const std::shared_ptr<bmcweb::AsyncResp>& asyncResp, const std::string& id,
+ const boost::system::error_code& ec, const sdbusplus::message_t& msg)
+{
+ if (ec == boost::system::errc::invalid_argument)
+ {
+ const sd_bus_error* errorMessage = msg.get_error();
+ if (errorMessage != nullptr)
+ {
+ for (const auto& arg : {"Id", "ReportActions"})
+ {
+ if (!handleParamError(asyncResp->res, errorMessage->message,
+ arg))
+ {
+ return;
+ }
+ }
+ }
+ }
+
+ if (!verifyCommonErrors(asyncResp->res, id, ec))
+ {
+ return;
+ }
+
+ messages::internalError(asyncResp->res);
}
inline void setReportActions(
@@ -980,11 +1170,9 @@
}
crow::connections::systemBus->async_method_call(
- [asyncResp, id = std::string(id)](const boost::system::error_code& ec) {
- if (!verifyCommonErrors(asyncResp->res, id, ec))
- {
- return;
- }
+ [asyncResp, id = std::string(id)](const boost::system::error_code& ec,
+ const sdbusplus::message_t& msg) {
+ afterSetReportActions(asyncResp, id, ec, msg);
},
"xyz.openbmc_project.Telemetry", getDbusReportPath(id),
"org.freedesktop.DBus.Properties", "Set",
@@ -994,17 +1182,15 @@
inline void setReportMetrics(
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp, std::string_view id,
- std::span<nlohmann::json::object_t> metrics)
+ std::vector<nlohmann::json::object_t>&& metrics)
{
sdbusplus::asio::getAllProperties(
*crow::connections::systemBus, telemetry::service,
telemetry::getDbusReportPath(id), telemetry::reportInterface,
- [asyncResp, id = std::string(id),
- redfishMetrics = std::vector<nlohmann::json::object_t>(
- metrics.begin(), metrics.end())](
+ [asyncResp, id = std::string(id), redfishMetrics = std::move(metrics)](
boost::system::error_code ec,
const dbus::utility::DBusPropertiesMap& properties) mutable {
- if (!redfish::telemetry::verifyCommonErrors(asyncResp->res, id, ec))
+ if (!verifyCommonErrors(asyncResp->res, id, ec))
{
return;
}
@@ -1175,7 +1361,7 @@
if (metrics)
{
- setReportMetrics(asyncResp, id, *metrics);
+ setReportMetrics(asyncResp, id, std::move(*metrics));
}
}