Commit 1af6118d authored by Danny Hindson's avatar Danny Hindson
Browse files

Modify registerFeatureUsage in Usage Service to reduce misuse

Now accepts:
- an enum for feature type rather than a string eg Algorithm
- a vector of strings for the feature name instead of string (eg class1->method1)

Also updated pre-existing calls to registerFeatureUsage in C++ and Python
parent 7b5a75ff
......@@ -1756,10 +1756,12 @@ void Algorithm::reportCompleted(const double &duration,
*/
void Algorithm::registerFeatureUsage() const {
if (UsageService::Instance().isEnabled()) {
std::vector<std::string> featureName;
std::ostringstream oss;
oss << this->name() << ".v" << this->version();
UsageService::Instance().registerFeatureUsage("Algorithm", oss.str(),
isChild());
featureName.push_back(oss.str());
UsageService::Instance().registerFeatureUsage(FeatureType::Algorithm,
featureName, isChild());
}
}
......
......@@ -22,6 +22,11 @@
namespace Mantid {
namespace Kernel {
/** An enum specifying the 3 possible features types that can be logged in the
usage service
*/
enum class FeatureType { Algorithm, Interface, Feature };
/** UsageReporter : The Usage reporter is responsible for collating, and sending
all usage data.
This centralizes all the logic covering Usage Reporting including:
......@@ -36,15 +41,19 @@ namespace Kernel {
class FeatureUsage {
public:
/// Constructor
FeatureUsage(const std::string &type, const std::string &name,
FeatureUsage(const FeatureType &type, const std::vector<std::string> &name,
const bool internal);
bool operator<(const FeatureUsage &r) const;
::Json::Value asJson() const;
std::string type;
std::string name;
FeatureType type;
std::vector<std::string> name;
bool internal;
protected:
std::string featureTypeToString() const;
std::string nameToString() const;
};
class MANTID_KERNEL_DLL UsageServiceImpl {
......@@ -57,8 +66,12 @@ public:
void setInterval(const uint32_t seconds = 60);
/// Registers the Startup of Mantid
void registerStartup();
/// Registers the use of a feature in mantid
void registerFeatureUsage(const std::string &type, const std::string &name,
/// Registers the use of a feature in mantid. Just support a version using
/// vector for now rather than additionally providing a version that takes a
/// single string. This will prevent any old style usage with name="a->b"
/// sneaking through
void registerFeatureUsage(const FeatureType &type,
const std::vector<std::string> &name,
const bool internal);
/// Returns true if usage reporting is enabled
......
......@@ -15,9 +15,14 @@
#include "MantidKernel/ParaViewVersion.h"
#include <Poco/ActiveResult.h>
#include <Poco/String.h>
#include <json/json.h>
namespace {
constexpr auto SEPARATOR = "->";
}
namespace Mantid {
namespace Kernel {
......@@ -27,7 +32,8 @@ Kernel::Logger g_log("UsageServiceImpl");
//----------------------------------------------------------------------------------------------
/** FeatureUsage
*/
FeatureUsage::FeatureUsage(const std::string &type, const std::string &name,
FeatureUsage::FeatureUsage(const FeatureType &type,
const std::vector<std::string> &name,
const bool internal)
: type(type), name(name), internal(internal) {}
......@@ -51,11 +57,40 @@ bool FeatureUsage::operator<(const FeatureUsage &r) const {
return false;
}
/// Convert the stored feature type enum to a string
std::string FeatureUsage::featureTypeToString() const {
switch (type) {
case FeatureType::Algorithm:
return "Algorithm";
case FeatureType::Feature:
return "Feature";
case FeatureType::Interface:
return "Interface";
}
return "Unknown";
}
/// Convert the stored vector of strings to a string eg Class1->Method1
std::string FeatureUsage::nameToString() const {
std::string result = "";
for (std::string i : name) {
result = result + Poco::trim(i) + SEPARATOR;
}
size_t returnLength = result.length() - std::string(SEPARATOR).length();
if (returnLength > 0) {
result = result.substr(0, returnLength);
}
return result;
}
::Json::Value FeatureUsage::asJson() const {
::Json::Value retVal;
retVal["type"] = type;
retVal["name"] = name;
retVal["type"] = featureTypeToString();
retVal["name"] = nameToString();
retVal["internal"] = internal;
return retVal;
......@@ -106,9 +141,9 @@ void UsageServiceImpl::registerStartup() {
/** registerFeatureUsage
*/
void UsageServiceImpl::registerFeatureUsage(const std::string &type,
const std::string &name,
const bool internal) {
void UsageServiceImpl::registerFeatureUsage(
const FeatureType &type, const std::vector<std::string> &name,
const bool internal) {
if (isEnabled()) {
std::lock_guard<std::mutex> _lock(m_mutex);
m_FeatureQueue.push(FeatureUsage(type, name, internal));
......
......@@ -86,12 +86,18 @@ public:
TestableUsageService usageService;
usageService.setInterval(10000);
usageService.setEnabled(true);
usageService.registerFeatureUsage("Algorithm", "MyAlg.v1", true);
usageService.registerFeatureUsage("Interface", "MyAlg.v1", true);
for (size_t i = 0; i < 10000; i++) {
usageService.registerFeatureUsage("Algorithm", "MyLoopAlg.v1", false);
}
usageService.registerFeatureUsage("Algorithm", "MyLoopAlg.v1", true);
usageService.registerFeatureUsage(Mantid::Kernel::FeatureType::Algorithm,
{"MyAlg.v1"}, true);
usageService.registerFeatureUsage(Mantid::Kernel::FeatureType::Interface,
{"MyAlg.v1"}, true);
/*for (size_t i = 0; i < 10000; i++) {
usageService.registerFeatureUsage(Mantid::Kernel::FeatureType::Algorithm,
{"MyLoopAlg.v1"}, false);
}*/
usageService.registerFeatureUsage(Mantid::Kernel::FeatureType::Algorithm,
{"MyLoopAlg.v1"}, true);
usageService.registerFeatureUsage(Mantid::Kernel::FeatureType::Algorithm,
{"MyAlg.v1", "Method1"}, true);
std::string message = usageService.generateFeatureUsageMessage();
......@@ -127,6 +133,9 @@ public:
if (type == "Algorithm" && name == "MyLoopAlg.v1" && internal == true &&
count == 1)
correct = true;
if (type == "Algorithm" && name == "MyAlg.v1->Method1" &&
internal == true && count == 1)
correct = true;
TSM_ASSERT("Usage record was not as expected", correct)
}
}
......@@ -136,7 +145,8 @@ public:
usageService.setInterval(10000);
usageService.setEnabled(true);
for (size_t i = 0; i < 10; i++) {
usageService.registerFeatureUsage("Algorithm", "MyLoopAlg.v1", false);
usageService.registerFeatureUsage(Mantid::Kernel::FeatureType::Algorithm,
{"MyLoopAlg.v1"}, false);
}
// this should empty the feature usage list
usageService.flush();
......@@ -149,7 +159,8 @@ public:
usageService.setInterval(10000);
usageService.setEnabled(true);
for (size_t i = 0; i < 10; i++) {
usageService.registerFeatureUsage("Algorithm", "MyLoopAlg.v1", false);
usageService.registerFeatureUsage(Mantid::Kernel::FeatureType::Algorithm,
{"MyLoopAlg.v1"}, false);
}
// this should empty the feature usage list
usageService.shutdown();
......
......@@ -5,8 +5,10 @@
// & Institut Laue - Langevin
// SPDX - License - Identifier: GPL - 3.0 +
#include "MantidKernel/UsageService.h"
#include "MantidPythonInterface/core/Converters/PySequenceToVector.h"
#include "MantidPythonInterface/core/GetPointer.h"
#include <boost/python/class.hpp>
#include <boost/python/enum.hpp>
#include <boost/python/reference_existing_object.hpp>
#include <mutex>
......@@ -14,6 +16,8 @@
using Mantid::Kernel::UsageService;
using Mantid::Kernel::UsageServiceImpl;
using namespace boost::python;
using ExtractStdString = boost::python::extract<std::string>;
using Mantid::PythonInterface::Converters::PySequenceToVector;
GET_POINTER_SPECIALIZATION(UsageServiceImpl)
......@@ -37,9 +41,24 @@ UsageServiceImpl &instance() {
});
return svc;
}
/// Register feature usage from a python list
void registerFeatureUsage(UsageServiceImpl &self,
const Mantid::Kernel::FeatureType &type,
const object &paths, const bool internal) {
ExtractStdString singleString(paths);
self.registerFeatureUsage(type, PySequenceToVector<std::string>(paths)(),
internal);
}
} // namespace
void export_UsageService() {
enum_<Mantid::Kernel::FeatureType>("FeatureType")
.value("Algorithm", Mantid::Kernel::FeatureType::Algorithm)
.value("Interface", Mantid::Kernel::FeatureType::Interface)
.value("Feature", Mantid::Kernel::FeatureType::Feature)
.export_values();
class_<UsageServiceImpl, boost::noncopyable>("UsageServiceImpl", no_init)
.def("flush", &UsageServiceImpl::flush, arg("self"),
......@@ -64,7 +83,7 @@ void export_UsageService() {
arg("self"), "Gets the application name that has invoked Mantid.")
.def("registerStartup", &UsageServiceImpl::registerStartup, arg("self"),
"Registers the startup of Mantid.")
.def("registerFeatureUsage", &UsageServiceImpl::registerFeatureUsage,
.def("registerFeatureUsage", &registerFeatureUsage,
(arg("self"), arg("type"), arg("name"), arg("internal")),
"Registers the use of a feature in Mantid.")
.def("getStartTime", &UsageServiceImpl::getStartTime, (arg("self")),
......
......@@ -8,7 +8,7 @@ from __future__ import (absolute_import, division, print_function, unicode_liter
import unittest
from mantid.kernel import (UsageService, UsageServiceImpl)
from mantid.kernel import (UsageService, UsageServiceImpl, FeatureType)
class UsageServiceTest(unittest.TestCase):
......@@ -43,7 +43,9 @@ class UsageServiceTest(unittest.TestCase):
def test_registerFeatureUsage(self):
UsageService.setEnabled(False)
#this will do nothing as it is disabled
UsageService.registerFeatureUsage("Algorithm", "Test.v1", True)
UsageService.registerFeatureUsage(FeatureType.Algorithm, ["testv1"], True)
UsageService.setEnabled(True)
UsageService.registerFeatureUsage(FeatureType.Algorithm, ["testv1"], True)
def test_Flush(self):
......
......@@ -41,7 +41,7 @@ InstrumentWindow::InstrumentWindow(const QString &wsName, const QString &label,
connect(m_instrumentWidget, SIGNAL(clearingHandle()), this,
SLOT(closeSafely()));
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Interface", "InstrumentView", false);
Mantid::Kernel::FeatureType::Interface, {"InstrumentView"}, false);
}
InstrumentWindow::~InstrumentWindow() {}
......
......@@ -309,7 +309,8 @@ ProjectRecovery::~ProjectRecovery() {
void ProjectRecovery::attemptRecovery() {
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Feature", "ProjectRecovery->AttemptRecovery", true);
Mantid::Kernel::FeatureType::Feature,
{"ProjectRecovery", "AttemptRecovery"}, true);
m_recoveryGui = new ProjectRecoveryPresenter(this, m_windowPtr);
bool failed = m_recoveryGui->startRecoveryView();
......
......@@ -21,7 +21,7 @@ ProjectRecoveryView::ProjectRecoveryView(QWidget *parent,
// Set the table information
addDataToTable();
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Interface", "ProjectRecoveryWindow", true);
Mantid::Kernel::FeatureType::Interface, {"ProjectRecoveryWindow"}, true);
}
void ProjectRecoveryView::addDataToTable() {
......@@ -34,28 +34,32 @@ void ProjectRecoveryView::onClickLastCheckpoint() {
// Recover last checkpoint
m_presenter->recoverLast();
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Feature", "ProjectRecoveryWindow->RecoverLastCheckpoint", false);
Mantid::Kernel::FeatureType::Feature,
{"ProjectRecoveryWindow", "RecoverLastCheckpoint"}, false);
}
void ProjectRecoveryView::onClickOpenLastInScriptWindow() {
// Open checkpoint in script window
m_presenter->openLastInEditor();
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Feature", "ProjectRecoveryWindow->OpenInScriptWindow", false);
Mantid::Kernel::FeatureType::Feature,
{"ProjectRecoveryWindow", "OpenInScriptWindow"}, false);
}
void ProjectRecoveryView::onClickStartMantidNormally() {
// Start save and close this, clear checkpoint that was offered for load
m_presenter->startMantidNormally();
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Feature", "ProjectRecoveryWindow->StartMantidNormally", false);
Mantid::Kernel::FeatureType::Feature,
{"ProjectRecoveryWindow", "StartMantidNormally"}, false);
}
void ProjectRecoveryView::reject() {
// Do the same as startMantidNormally
m_presenter->startMantidNormally();
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Feature", "ProjectRecoveryWindow->StartMantidNormally", false);
Mantid::Kernel::FeatureType::Feature,
{"ProjectRecoveryWindow", "StartMantidNormally"}, false);
}
void ProjectRecoveryView::updateProgressBar(int newValue, bool err) {
......
......@@ -22,7 +22,8 @@ RecoveryFailureView::RecoveryFailureView(QWidget *parent,
// Set the table information
addDataToTable();
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Interface", "ProjectRecoveryFailureWindow", true);
Mantid::Kernel::FeatureType::Interface, {"ProjectRecoveryFailureWindow"},
true);
}
void RecoveryFailureView::addDataToTable() {
......@@ -41,7 +42,8 @@ void RecoveryFailureView::onClickLastCheckpoint() {
// Recover last checkpoint
m_presenter->recoverLast();
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Feature", "ProjectRecoveryFailureWindow->RecoverLastCheckpoint", false);
Mantid::Kernel::FeatureType::Feature,
{"ProjectRecoveryFailureWindow", "RecoverLastCheckpoint"}, false);
}
void RecoveryFailureView::onClickSelectedCheckpoint() {
......@@ -55,8 +57,8 @@ void RecoveryFailureView::onClickSelectedCheckpoint() {
m_presenter->recoverSelectedCheckpoint(text);
}
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Feature", "ProjectRecoveryFailureWindow->RecoverSelectedCheckpoint",
false);
Mantid::Kernel::FeatureType::Feature,
{"ProjectRecoveryFailureWindow", "RecoverSelectedCheckpoint"}, false);
}
void RecoveryFailureView::onClickOpenSelectedInScriptWindow() {
......@@ -70,22 +72,24 @@ void RecoveryFailureView::onClickOpenSelectedInScriptWindow() {
m_presenter->openSelectedInEditor(text);
}
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Feature", "ProjectRecoveryFailureWindow->OpenSelectedInScriptWindow",
false);
Mantid::Kernel::FeatureType::Feature,
{"ProjectRecoveryFailureWindow", "OpenSelectedInScriptWindow"}, false);
}
void RecoveryFailureView::onClickStartMantidNormally() {
// Start save and close this, clear checkpoint that was offered for load
m_presenter->startMantidNormally();
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Feature", "ProjectRecoveryFailureWindow->StartMantidNormally", false);
Mantid::Kernel::FeatureType::Feature,
{"ProjectRecoveryFailureWindow", "StartMantidNormally"}, false);
}
void RecoveryFailureView::reject() {
// Do nothing just absorb request
m_presenter->startMantidNormally();
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Feature", "ProjectRecoveryFailureWindow->StartMantidNormally", false);
Mantid::Kernel::FeatureType::Feature,
{"ProjectRecoveryFailureWindow", "StartMantidNormally"}, false);
}
void RecoveryFailureView::updateProgressBar(const int newValue,
......
......@@ -352,8 +352,8 @@ ViewBase *MdViewerWidget::createAndSetMainViewWidget(QWidget *container,
view->setColorScaleLock(&m_colorScaleLock);
using Mantid::Kernel::UsageService;
UsageService::Instance().registerFeatureUsage("Interface", featureName,
false);
UsageService::Instance().registerFeatureUsage(
Mantid::Kernel::FeatureType::Interface, {featureName}, false);
return view;
}
......
......@@ -39,7 +39,8 @@ BackgroundRemover::BackgroundRemover(QWidget *parent)
: UserSubWindow{parent}, m_sliceSelector(), m_inputDataControl(),
m_displayControl(), m_fitControl{nullptr}, m_fourierTransform{nullptr} {
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Interface", "DynamicPDF->BackgroundRemover", false);
Mantid::Kernel::FeatureType::Interface,
{"DynamicPDF", "BackgroundRemover"}, false);
}
/**
......
......@@ -71,7 +71,8 @@ SliceSelector::SliceSelector(QWidget *parent)
m_loadedWorkspace(), m_selectedWorkspaceIndex{0} {
this->observePreDelete(true); // Subscribe to notifications
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Feature", "DynamicPDF->SliceSelector", false);
Mantid::Kernel::FeatureType::Feature, {"DynamicPDF", "SliceSelector"},
false);
this->initLayout();
}
......
......@@ -24,20 +24,20 @@ public:
TrackedAction(const QIcon &icon, const QString &text, QObject *parent);
virtual ~TrackedAction() = default;
void setTrackingName(const std::string &name);
std::string getTrackingName() const;
void setTrackingName(const std::vector<std::string> &name);
std::vector<std::string> getTrackingName() const;
void setIsTracking(const bool enableTracking);
bool getIsTracking() const;
protected:
virtual std::string generateTrackingName() const;
virtual void registerUsage(const std::string &name);
virtual std::vector<std::string> generateTrackingName() const;
virtual void registerUsage(const std::vector<std::string> &name);
private:
void setupTracking();
bool m_isTracking;
mutable std::string m_trackingName;
mutable std::vector<std::string> m_trackingName;
public slots:
void trackActivation(const bool checked);
......
......@@ -42,7 +42,7 @@ TrackedAction::TrackedAction(const QIcon &icon, const QString &text,
/** Sets the tracking name for this action
* @param name the tracking name for this action
**/
void TrackedAction::setTrackingName(const std::string &name) {
void TrackedAction::setTrackingName(const std::vector<std::string> &name) {
m_trackingName = name;
}
......@@ -51,7 +51,7 @@ void TrackedAction::setTrackingName(const std::string &name) {
* generateTrackingName
* @returns The tracking name for this action
**/
std::string TrackedAction::getTrackingName() const {
std::vector<std::string> TrackedAction::getTrackingName() const {
if (m_trackingName.empty()) {
m_trackingName = generateTrackingName();
}
......@@ -79,9 +79,9 @@ void TrackedAction::setupTracking() {
/** Creates a tracking name from the action text
* @returns A generated name using ApplicationName->ActionText
**/
std::string TrackedAction::generateTrackingName() const {
return QCoreApplication::applicationName().toStdString() + "->" +
QAction::text().remove("&").remove(" ").toStdString();
std::vector<std::string> TrackedAction::generateTrackingName() const {
return {QCoreApplication::applicationName().toStdString(),
QAction::text().remove("&").remove(" ").toStdString()};
}
/** Registers the feature usage if usage is enabled
......@@ -91,16 +91,16 @@ void TrackedAction::trackActivation(const bool checked) {
UNUSED_ARG(checked);
if (m_isTracking) {
// do tracking
registerUsage(getTrackingName());
registerUsage({getTrackingName()});
}
}
/** Registers the feature usage with the usage service
* @param name The name to use when registering usage
**/
void TrackedAction::registerUsage(const std::string &name) {
Mantid::Kernel::UsageService::Instance().registerFeatureUsage("Feature", name,
false);
void TrackedAction::registerUsage(const std::vector<std::string> &name) {
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
Mantid::Kernel::FeatureType::Feature, name, false);
}
} // namespace MantidWidgets
} // namespace MantidQt
......@@ -56,7 +56,8 @@ void UserSubWindow::initializeLayout() {
m_bIsInitialized = true;
Mantid::Kernel::UsageService::Instance().registerFeatureUsage(
"Interface", m_ifacename.toStdString(), false);
Mantid::Kernel::FeatureType::Interface, {m_ifacename.toStdString()},
false);
}
/**
......
......@@ -26,13 +26,15 @@ class TrackedActionTest : public CxxTest::TestSuite {
QObject *parent)
: TrackedAction(icon, text, parent), m_lastName(){};
std::string getLastUsedName() const { return m_lastName; };
std::vector<std::string> getLastUsedName() const { return m_lastName; };
protected:
void registerUsage(const std::string &name) override { m_lastName = name; };
void registerUsage(const std::vector<std::string> &name) override {
m_lastName = name;
};
private:
std::string m_lastName;
std::vector<std::string> m_lastName;
};
public:
......@@ -55,12 +57,18 @@ public:
TestableTrackedAction action(QString::fromStdString("TestName"), &parent);
std::string appNamePrefix =
QCoreApplication::applicationName().toStdString() + "->";
QCoreApplication::applicationName().toStdString();
TS_ASSERT_EQUALS(action.getTrackingName(),
appNamePrefix + "TestName"); // default state
action.setTrackingName("TestName2");
TS_ASSERT_EQUALS(action.getTrackingName(), "TestName2"); // altered state
TS_ASSERT_EQUALS(action.getTrackingName().size(), 2);
TS_ASSERT_EQUALS(action.getTrackingName()[0],
appNamePrefix); // default state
TS_ASSERT_EQUALS(action.getTrackingName()[1], "TestName"); // default state
action.setTrackingName({"TestName2"});
TS_ASSERT_EQUALS(action.getTrackingName().size(), 1);
TS_ASSERT_EQUALS(action.getTrackingName()[0],
"TestName2"); // altered state
}
void testTrackingCallLogic() {
......@@ -68,17 +76,19 @@ public:
TestableTrackedAction action(QString::fromStdString("TestName"), &parent);
// tracking should be on by default
TS_ASSERT_EQUALS(action.getIsTracking(), true); // default state
TS_ASSERT_EQUALS(action.getLastUsedName(), ""); // default state
TS_ASSERT_EQUALS(action.getIsTracking(), true); // default state
TS_ASSERT_EQUALS(action.getLastUsedName().empty(), true); // default state
action.setTrackingName("ShouldTrack");
action.setTrackingName({"ShouldTrack"});
action.trigger();
TS_ASSERT_EQUALS(action.getLastUsedName(),
TS_ASSERT_EQUALS(action.getLastUsedName().size(), 1);
TS_ASSERT_EQUALS(action.getLastUsedName()[0],
"ShouldTrack"); // tracking occurred state
action.setIsTracking(false);
action.setTrackingName("ShouldNotTrack");
action.setTrackingName({"ShouldNotTrack"});
action.trigger();
TS_ASSERT_DIFFERS(action.getLastUsedName(),
TS_ASSERT_EQUALS(action.getLastUsedName().size(), 1);
TS_ASSERT_DIFFERS(action.getLastUsedName()[0],
"ShouldNotTrack"); // Should not have tracked