feat: add eventlogger.hpp header for DDE applications#185
feat: add eventlogger.hpp header for DDE applications#185Ivy233 merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's GuideAdds a header-only C++ event logger for DDE applications that dynamically loads libdeepin-event-log.so at runtime, wires its installation into the build system, and restricts logging to specific UOS editions by default. Sequence diagram for EventLogger initialization and loggingsequenceDiagram
actor App
participant EventLogger as DDE_EventLogger_EventLogger
participant DSysInfo as DSysInfo_uosEditionType
participant QLibrary as QLibrary_loader
participant Lib as libdeepin_event_log_so
App->>EventLogger: instance()
Note over EventLogger: Singleton constructor invoked on first call
EventLogger->>QLibrary: load(/usr/lib/libdeepin-event-log.so)
QLibrary-->>EventLogger: load result
EventLogger->>QLibrary: resolve(Initialize)
QLibrary-->>EventLogger: Initialize*
EventLogger->>QLibrary: resolve(WriteEventLog)
QLibrary-->>EventLogger: WriteEventLog*
App->>EventLogger: init(package_id, enable_sig)
EventLogger->>DSysInfo: uosEditionType()
DSysInfo-->>EventLogger: edition_type
alt shouldEnableEventLog() is false
EventLogger-->>App: false
else shouldEnableEventLog() is true
EventLogger->>EventLogger: check m_initialize,m_writeEventLog
alt function pointers missing
EventLogger-->>App: false
else function pointers available
EventLogger->>Lib: Initialize(package_id, enable_sig)
Lib-->>EventLogger: initialized_flag
EventLogger-->>App: initialized_flag
end
end
App->>EventLogger: writeEventLog(tid, target, key, value)
EventLogger->>DSysInfo: uosEditionType()
DSysInfo-->>EventLogger: edition_type
alt shouldEnableEventLog() is false
EventLogger-->>App: return
else shouldEnableEventLog() is true
EventLogger->>EventLogger: lock m_mutex
EventLogger->>EventLogger: check m_initialized
alt not initialized
EventLogger-->>App: return
else initialized
EventLogger->>EventLogger: structToJson(EventLoggerData)
EventLogger->>Lib: WriteEventLog(json_string)
Lib-->>EventLogger: ack
EventLogger-->>App: return
end
end
Class diagram for new DDE_EventLogger C++ APIclassDiagram
direction LR
class DDE_EventLogger {
}
namespace DDE_EventLogger {
class EventLoggerData {
+qint64 tid
+QString target
+QMap~QString,QString~ message
+EventLoggerData()
+EventLoggerData(qint64 tid, QString target, QMap~QString,QString~ message)
+EventLoggerData operator=(EventLoggerData data)
}
class EventLogger {
+static EventLogger &instance()
+void writeEventLog(EventLoggerData data)
+void writeEventLog(qint64 tid, QString target, QString key, QString value)
+bool init(QString package_id, bool enable_sig)
-EventLogger()
-~EventLogger()
-EventLogger(EventLogger other)
-EventLogger &operator=(EventLogger other)
-QJsonDocument structToJson(EventLoggerData data)
-std::mutex m_mutex
-bool m_initialized
-Initialize m_initialize
-WriteEventLog m_writeEventLog
-QLibrary m_library
}
class shouldEnableEventLog {
}
}
EventLoggerData <.. EventLogger : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since this is a header-only singleton used across multiple translation units, consider avoiding a non-trivial destructor with side effects (library unload) in the header to reduce ODR and static-destruction-order risks; you could rely on process teardown or wrap QLibrary in a separate .cpp-managed helper.
- The implementation mixes QLibrary with POSIX dynamic loading headers (<dlfcn.h>, <unistd.h>) and doesn’t use the latter, so it’s clearer and safer to remove the unused C headers or switch fully to one mechanism.
- Library loading and symbol resolution happen unconditionally in the constructor even when shouldEnableEventLog() would later disable logging; to minimize overhead and spurious errors on non-UosProfessional systems, consider deferring load/resolve until init() and guarding it with shouldEnableEventLog().
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since this is a header-only singleton used across multiple translation units, consider avoiding a non-trivial destructor with side effects (library unload) in the header to reduce ODR and static-destruction-order risks; you could rely on process teardown or wrap QLibrary in a separate .cpp-managed helper.
- The implementation mixes QLibrary with POSIX dynamic loading headers (<dlfcn.h>, <unistd.h>) and doesn’t use the latter, so it’s clearer and safer to remove the unused C headers or switch fully to one mechanism.
- Library loading and symbol resolution happen unconditionally in the constructor even when shouldEnableEventLog() would later disable logging; to minimize overhead and spurious errors on non-UosProfessional systems, consider deferring load/resolve until init() and guarding it with shouldEnableEventLog().Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
9ba869b to
a0a55d9
Compare
Add a C++ header-only event logger that can be used by DDE applications to log user behavior events. The logger uses dlopen to dynamically load libdeepin-event-log.so library at runtime. Features: - Header-only implementation, no linking required - Dynamic library loading with QLibrary - System edition check (only UosProfessional enabled by default) - Debug mode macro for testing on other editions - LGPL-3.0-or-later license for easy integration 添加用于DDE应用程序的事件日志头文件。日志记录器使用dlopen在运行时动态加载 libdeepin-event-log.so库。 Log: 添加事件日志头文件 PMS: TASK-388657
a0a55d9 to
bb041e9
Compare
deepin pr auto review感谢您分享的代码差异。我仔细审查了这些更改,主要涉及添加LGPL-3.0-or-later许可证文件、修改Makefile以安装C++头文件,以及新增一个事件日志记录器的C++头文件。以下是我的审查意见: 1. 语法逻辑审查eventlogger.hpp:
Makefile:
2. 代码质量审查eventlogger.hpp:
建议改进:
3. 代码性能审查eventlogger.hpp:
建议改进:
4. 代码安全审查eventlogger.hpp:
建议改进:
5. 其他审查意见许可证文件:
Makefile:
总体评价这些更改整体上是合理的,添加了事件日志记录功能,并正确地处理了许可证和安装问题。代码质量较高,但在安全性和性能方面有一些改进空间。建议在合并前考虑上述改进意见,特别是关于输入验证和库加载安全性的改进。 如果您需要更详细的代码示例来实现这些改进,或者有其他问题,请随时告诉我。 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ivy233, mhduiy The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Add a C++ header-only event logger that can be used by DDE applications to log user behavior events. The logger uses dlopen to dynamically load libdeepin-event-log.so library at runtime.
Features:
添加用于DDE应用程序的事件日志头文件。日志记录器使用dlopen在运行时动态加载
libdeepin-event-log.so库。
Log: 添加事件日志头文件
PMS: TASK-388657
Summary by Sourcery
Add a header-only C++ event logging utility for DDE applications and ensure it is installed as part of the development artifacts.
New Features:
Enhancements: