Performance/networking optimizations #45
Conversation
WalkthroughThis update introduces a new compile-time logger verbosity option, adds a logger wrapper for compile-time verbosity filtering, and propagates const-correctness and container type changes (from Changes
Sequence Diagram(s)sequenceDiagram
participant Config as CMake/Config
participant Main as main.cpp
participant Logger as LoggerWrapper
participant User as Developer/User
User->>Config: Set MODULE_GATEWAY_MINIMUM_LOGGER_VERBOSITY (string)
Config->>Main: Compile with macro MODULE_GATEWAY_MINIMUM_LOGGER_VERBOSITY
Main->>Logger: Logger::init(LOGGER_VERBOSITY)
Logger->>Logger: Compile-time filter for log level
Main->>Logger: Log messages
Logger-->>Main: Filtered log output (at or above set verbosity)
sequenceDiagram
participant ExternalClient
participant DummyCommunication
participant Settings
ExternalClient->>Settings: Read protocol type
alt protocol == DUMMY
ExternalClient->>DummyCommunication: Create and initialize
DummyCommunication->>ExternalClient: Provide dummy send/receive
else protocol == MQTT
ExternalClient->>MqttCommunication: Create and initialize
end
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
include/bringauto/structures/AtomicQueue.hpp (1)
33-36: Clarify return-value semantics in the fresh comment while you’re hereNice typo fix. Since you’re touching the docblock anyway, consider spelling out that the function returns
trueonly when the timeout expired and the queue is still empty:- * @return true if the queue is empty + * @return true if the queue is still empty after the timeout has elapsed, + * false if a value became available before the timeout.README.md (1)
80-88: LGTM! Clear documentation of new logger verbosity option.The documentation clearly explains the new
MINIMUM_LOGGER_VERBOSITYCMake option and its performance benefits through compile-time log filtering. This aligns well with the PR's performance optimization objectives.Consider fixing the minor formatting inconsistency flagged by markdownlint - the list uses asterisks while other lists in the file use dashes.
-* MINIMUM_LOGGER_VERBOSITY=0/1/2/3/4 +- MINIMUM_LOGGER_VERBOSITY=0/1/2/3/4source/bringauto/common_utils/EnumUtils.cpp (1)
13-15: DUMMY protocol support added correctly.The logic for handling the
DUMMYprotocol type is consistent with the existingMQTTprotocol handling. However, there's an indentation inconsistency - the new code uses spaces while the existing code appears to use tabs.Consider fixing the indentation to match the existing style:
- } else if(toEnum == settings::Constants::DUMMY) { - return structures::ProtocolType::DUMMY; - } + } else if(toEnum == settings::Constants::DUMMY) { + return structures::ProtocolType::DUMMY; + }
include/bringauto/external_client/connection/communication/DummyCommunication.hpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
80-82: Markdown-lint MD004 violation – switch to dash bullets for consistency
markdownlintflags the new bullet because the project’s doc style-guide prefers-over*.
Since the surrounding list items (61-78) already use*, you can either:
- Adopt dashes here and everywhere else (preferred ‑ silences the linter globally), or
- Add an explicit rule override to keep asterisks.
Example fix for the new item:
-* MODULE_GATEWAY_MINIMUM_LOGGER_VERBOSITY=0/1/2/3/4 +- MODULE_GATEWAY_MINIMUM_LOGGER_VERBOSITY=0/1/2/3/4
83-88: Spell out the enum names to avoid “magic numbers”Readers unfamiliar with
ba-loggermay not know which numeric level maps to which macro/enum.
Consider appending the symbolic names once to improve clarity:- - 0: DEBUG - - 1: INFO - - 2: WARNING - - 3: ERROR - - 4: CRITICAL + - 0 (`BA_LOGGER_VERBOSITY_DEBUG`): DEBUG + - 1 (`BA_LOGGER_VERBOSITY_INFO`): INFO + - 2 (`BA_LOGGER_VERBOSITY_WARNING`): WARNING + - 3 (`BA_LOGGER_VERBOSITY_ERROR`): ERROR + - 4 (`BA_LOGGER_VERBOSITY_CRITICAL`): CRITICALThis keeps the README self-contained and reduces the mental jump to the codebase/macros.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CMakeLists.txt(1 hunks)Dockerfile(1 hunks)README.md(1 hunks)include/bringauto/settings/LoggerId.hpp(1 hunks)include/bringauto/settings/LoggerWrapper.hpp(1 hunks)include/bringauto/structures/Connection.hpp(1 hunks)main.cpp(3 hunks)source/bringauto/common_utils/ProtobufUtils.cpp(5 hunks)source/bringauto/internal_server/InternalServer.cpp(18 hunks)
🧠 Learnings (1)
README.md (6)
Learnt from: koudis
PR: #43
File: CMakeLists.txt:11-14
Timestamp: 2025-06-29T18:12:34.666Z
Learning: In the ModuleGateway project's CMakeLists.txt, compile definitions for version strings are intentionally set with escaped quotes using CMDEF_COMPILE_DEFINITIONS to produce compiler flags like -DMODULE_GATEWAY_VERSION="1.3.3", ensuring the macro expands to a proper string literal in C++ code.
Learnt from: MarioIvancik
PR: #38
File: include/bringauto/structures/LoggingSettings.hpp:10-15
Timestamp: 2024-10-31T10:45:48.494Z
Learning: In the ModuleGateway project, values in the structures defined in include/bringauto/structures/LoggingSettings.hpp are validated in SettingsParser.cpp, so additional validation within the structures is unnecessary.
Learnt from: Mayt34
PR: bringauto/transparent-module#1
File: CMakeLists.txt:15-20
Timestamp: 2024-10-03T17:41:56.410Z
Learning: In the transparent_module project, all modules should be built by default; FLEET_PROTOCOL_BUILD_MODULE_GATEWAY and FLEET_PROTOCOL_BUILD_EXTERNAL_SERVER should be set to ON by default.
Learnt from: Mayt34
PR: bringauto/transparent-module#1
File: CMakeLists.txt:15-20
Timestamp: 2024-10-09T05:03:50.249Z
Learning: In the transparent_module project, all modules should be built by default; FLEET_PROTOCOL_BUILD_MODULE_GATEWAY and FLEET_PROTOCOL_BUILD_EXTERNAL_SERVER should be set to ON by default.
Learnt from: mikusaq
PR: bacpack-system/packager#24
File: modules/bringauto_log/Handler.go:40-54
Timestamp: 2024-10-09T05:03:50.249Z
Learning: The bringauto_log module does not currently use the debug log level.
Learnt from: mikusaq
PR: bacpack-system/packager#24
File: modules/bringauto_log/Handler.go:40-54
Timestamp: 2024-09-30T11:00:40.818Z
Learning: The bringauto_log module does not currently use the debug log level.
🪛 markdownlint-cli2 (0.17.2)
README.md
80-80: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
✅ Files skipped from review due to trivial changes (1)
- Dockerfile
🚧 Files skipped from review as they are similar to previous changes (7)
- include/bringauto/structures/Connection.hpp
- main.cpp
- source/bringauto/common_utils/ProtobufUtils.cpp
- CMakeLists.txt
- include/bringauto/settings/LoggerWrapper.hpp
- include/bringauto/settings/LoggerId.hpp
- source/bringauto/internal_server/InternalServer.cpp
🧰 Additional context used
🧠 Learnings (1)
README.md (6)
Learnt from: koudis
PR: #43
File: CMakeLists.txt:11-14
Timestamp: 2025-06-29T18:12:34.666Z
Learning: In the ModuleGateway project's CMakeLists.txt, compile definitions for version strings are intentionally set with escaped quotes using CMDEF_COMPILE_DEFINITIONS to produce compiler flags like -DMODULE_GATEWAY_VERSION="1.3.3", ensuring the macro expands to a proper string literal in C++ code.
Learnt from: MarioIvancik
PR: #38
File: include/bringauto/structures/LoggingSettings.hpp:10-15
Timestamp: 2024-10-31T10:45:48.494Z
Learning: In the ModuleGateway project, values in the structures defined in include/bringauto/structures/LoggingSettings.hpp are validated in SettingsParser.cpp, so additional validation within the structures is unnecessary.
Learnt from: Mayt34
PR: bringauto/transparent-module#1
File: CMakeLists.txt:15-20
Timestamp: 2024-10-03T17:41:56.410Z
Learning: In the transparent_module project, all modules should be built by default; FLEET_PROTOCOL_BUILD_MODULE_GATEWAY and FLEET_PROTOCOL_BUILD_EXTERNAL_SERVER should be set to ON by default.
Learnt from: Mayt34
PR: bringauto/transparent-module#1
File: CMakeLists.txt:15-20
Timestamp: 2024-10-09T05:03:50.249Z
Learning: In the transparent_module project, all modules should be built by default; FLEET_PROTOCOL_BUILD_MODULE_GATEWAY and FLEET_PROTOCOL_BUILD_EXTERNAL_SERVER should be set to ON by default.
Learnt from: mikusaq
PR: bacpack-system/packager#24
File: modules/bringauto_log/Handler.go:40-54
Timestamp: 2024-10-09T05:03:50.249Z
Learning: The bringauto_log module does not currently use the debug log level.
Learnt from: mikusaq
PR: bacpack-system/packager#24
File: modules/bringauto_log/Handler.go:40-54
Timestamp: 2024-09-30T11:00:40.818Z
Learning: The bringauto_log module does not currently use the debug log level.
🪛 markdownlint-cli2 (0.17.2)
README.md
80-80: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
include/bringauto/settings/LoggerId.hpp (1)
34-45: Consider making the DEBUG case explicit.The function correctly converts string verbosity levels to enum values. However, the "DEBUG" case is handled implicitly by the default return. Consider making it explicit for clarity:
constexpr logging::LoggerVerbosity toLoggerVerbosity(std::string_view verbosityString) { - if (verbosityString == "INFO") { + if (verbosityString == "DEBUG") { + return logging::LoggerVerbosity::Debug; + } else if (verbosityString == "INFO") { return logging::LoggerVerbosity::Info; } else if (verbosityString == "WARNING") { return logging::LoggerVerbosity::Warning; } else if (verbosityString == "ERROR") { return logging::LoggerVerbosity::Error; } else if (verbosityString == "CRITICAL") { return logging::LoggerVerbosity::Critical; } - return logging::LoggerVerbosity::Debug; + // This should never be reached due to static_assert validation + return logging::LoggerVerbosity::Debug; }README.md (1)
80-83: Good documentation of the new compile-time logging feature.The documentation accurately describes the new
MODULE_GATEWAY_MINIMUM_LOGGER_VERBOSITYCMake argument and its performance benefits. The listed verbosity levels match the implementation.Fix the list style for consistency with the rest of the document:
-* MODULE_GATEWAY_MINIMUM_LOGGER_VERBOSITY=DEBUG/INFO/WARNING/ERROR/CRITICAL +- MODULE_GATEWAY_MINIMUM_LOGGER_VERBOSITY=DEBUG/INFO/WARNING/ERROR/CRITICAL
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CMakeLists.txt(1 hunks)README.md(1 hunks)include/bringauto/external_client/connection/communication/DummyCommunication.hpp(1 hunks)include/bringauto/settings/LoggerId.hpp(1 hunks)
🧠 Learnings (2)
include/bringauto/settings/LoggerId.hpp (5)
Learnt from: MarioIvancik
PR: #38
File: include/bringauto/structures/LoggingSettings.hpp:10-15
Timestamp: 2024-10-31T10:45:48.494Z
Learning: In the ModuleGateway project, values in the structures defined in include/bringauto/structures/LoggingSettings.hpp are validated in SettingsParser.cpp, so additional validation within the structures is unnecessary.
Learnt from: MarioIvancik
PR: #38
File: include/bringauto/structures/LoggingSettings.hpp:26-31
Timestamp: 2024-10-31T10:44:39.657Z
Learning: In the ModuleGateway project, logging settings are validated in SettingsParser, so additional validation in the LoggingSettings structure is not necessary.
Learnt from: koudis
PR: #43
File: CMakeLists.txt:11-14
Timestamp: 2025-06-29T18:12:34.666Z
Learning: In the ModuleGateway project's CMakeLists.txt, compile definitions for version strings are intentionally set with escaped quotes using CMDEF_COMPILE_DEFINITIONS to produce compiler flags like -DMODULE_GATEWAY_VERSION="1.3.3", ensuring the macro expands to a proper string literal in C++ code.
Learnt from: Melky-Phoe
PR: bringauto/fleet-management-http-api#17
File: fleet_management_api/main.py:42-42
Timestamp: 2024-09-18T06:54:20.565Z
Learning: The team prefers using constants for configuration values like the logging configuration file path instead of making them configurable via command-line arguments or configuration parameters unless necessary.
Learnt from: Melky-Phoe
PR: bringauto/fleet-management-http-api#17
File: fleet_management_api/main.py:42-42
Timestamp: 2024-10-09T05:03:50.249Z
Learning: The team prefers using constants for configuration values like the logging configuration file path instead of making them configurable via command-line arguments or configuration parameters unless necessary.
README.md (6)
Learnt from: koudis
PR: #43
File: CMakeLists.txt:11-14
Timestamp: 2025-06-29T18:12:34.666Z
Learning: In the ModuleGateway project's CMakeLists.txt, compile definitions for version strings are intentionally set with escaped quotes using CMDEF_COMPILE_DEFINITIONS to produce compiler flags like -DMODULE_GATEWAY_VERSION="1.3.3", ensuring the macro expands to a proper string literal in C++ code.
Learnt from: MarioIvancik
PR: #38
File: include/bringauto/structures/LoggingSettings.hpp:10-15
Timestamp: 2024-10-31T10:45:48.494Z
Learning: In the ModuleGateway project, values in the structures defined in include/bringauto/structures/LoggingSettings.hpp are validated in SettingsParser.cpp, so additional validation within the structures is unnecessary.
Learnt from: mikusaq
PR: bacpack-system/packager#24
File: modules/bringauto_log/Handler.go:40-54
Timestamp: 2024-10-09T05:03:50.249Z
Learning: The bringauto_log module does not currently use the debug log level.
Learnt from: mikusaq
PR: bacpack-system/packager#24
File: modules/bringauto_log/Handler.go:40-54
Timestamp: 2024-09-30T11:00:40.818Z
Learning: The bringauto_log module does not currently use the debug log level.
Learnt from: Mayt34
PR: bringauto/transparent-module#1
File: CMakeLists.txt:15-20
Timestamp: 2024-10-03T17:41:56.410Z
Learning: In the transparent_module project, all modules should be built by default; FLEET_PROTOCOL_BUILD_MODULE_GATEWAY and FLEET_PROTOCOL_BUILD_EXTERNAL_SERVER should be set to ON by default.
Learnt from: Mayt34
PR: bringauto/transparent-module#1
File: CMakeLists.txt:15-20
Timestamp: 2024-10-09T05:03:50.249Z
Learning: In the transparent_module project, all modules should be built by default; FLEET_PROTOCOL_BUILD_MODULE_GATEWAY and FLEET_PROTOCOL_BUILD_EXTERNAL_SERVER should be set to ON by default.
🪛 markdownlint-cli2 (0.17.2)
README.md
80-80: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
🚧 Files skipped from review as they are similar to previous changes (2)
- CMakeLists.txt
- include/bringauto/external_client/connection/communication/DummyCommunication.hpp
🧰 Additional context used
🧠 Learnings (2)
include/bringauto/settings/LoggerId.hpp (5)
Learnt from: MarioIvancik
PR: #38
File: include/bringauto/structures/LoggingSettings.hpp:10-15
Timestamp: 2024-10-31T10:45:48.494Z
Learning: In the ModuleGateway project, values in the structures defined in include/bringauto/structures/LoggingSettings.hpp are validated in SettingsParser.cpp, so additional validation within the structures is unnecessary.
Learnt from: MarioIvancik
PR: #38
File: include/bringauto/structures/LoggingSettings.hpp:26-31
Timestamp: 2024-10-31T10:44:39.657Z
Learning: In the ModuleGateway project, logging settings are validated in SettingsParser, so additional validation in the LoggingSettings structure is not necessary.
Learnt from: koudis
PR: #43
File: CMakeLists.txt:11-14
Timestamp: 2025-06-29T18:12:34.666Z
Learning: In the ModuleGateway project's CMakeLists.txt, compile definitions for version strings are intentionally set with escaped quotes using CMDEF_COMPILE_DEFINITIONS to produce compiler flags like -DMODULE_GATEWAY_VERSION="1.3.3", ensuring the macro expands to a proper string literal in C++ code.
Learnt from: Melky-Phoe
PR: bringauto/fleet-management-http-api#17
File: fleet_management_api/main.py:42-42
Timestamp: 2024-09-18T06:54:20.565Z
Learning: The team prefers using constants for configuration values like the logging configuration file path instead of making them configurable via command-line arguments or configuration parameters unless necessary.
Learnt from: Melky-Phoe
PR: bringauto/fleet-management-http-api#17
File: fleet_management_api/main.py:42-42
Timestamp: 2024-10-09T05:03:50.249Z
Learning: The team prefers using constants for configuration values like the logging configuration file path instead of making them configurable via command-line arguments or configuration parameters unless necessary.
README.md (6)
Learnt from: koudis
PR: #43
File: CMakeLists.txt:11-14
Timestamp: 2025-06-29T18:12:34.666Z
Learning: In the ModuleGateway project's CMakeLists.txt, compile definitions for version strings are intentionally set with escaped quotes using CMDEF_COMPILE_DEFINITIONS to produce compiler flags like -DMODULE_GATEWAY_VERSION="1.3.3", ensuring the macro expands to a proper string literal in C++ code.
Learnt from: MarioIvancik
PR: #38
File: include/bringauto/structures/LoggingSettings.hpp:10-15
Timestamp: 2024-10-31T10:45:48.494Z
Learning: In the ModuleGateway project, values in the structures defined in include/bringauto/structures/LoggingSettings.hpp are validated in SettingsParser.cpp, so additional validation within the structures is unnecessary.
Learnt from: mikusaq
PR: bacpack-system/packager#24
File: modules/bringauto_log/Handler.go:40-54
Timestamp: 2024-10-09T05:03:50.249Z
Learning: The bringauto_log module does not currently use the debug log level.
Learnt from: mikusaq
PR: bacpack-system/packager#24
File: modules/bringauto_log/Handler.go:40-54
Timestamp: 2024-09-30T11:00:40.818Z
Learning: The bringauto_log module does not currently use the debug log level.
Learnt from: Mayt34
PR: bringauto/transparent-module#1
File: CMakeLists.txt:15-20
Timestamp: 2024-10-03T17:41:56.410Z
Learning: In the transparent_module project, all modules should be built by default; FLEET_PROTOCOL_BUILD_MODULE_GATEWAY and FLEET_PROTOCOL_BUILD_EXTERNAL_SERVER should be set to ON by default.
Learnt from: Mayt34
PR: bringauto/transparent-module#1
File: CMakeLists.txt:15-20
Timestamp: 2024-10-09T05:03:50.249Z
Learning: In the transparent_module project, all modules should be built by default; FLEET_PROTOCOL_BUILD_MODULE_GATEWAY and FLEET_PROTOCOL_BUILD_EXTERNAL_SERVER should be set to ON by default.
🪛 markdownlint-cli2 (0.17.2)
README.md
80-80: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
🔇 Additional comments (4)
include/bringauto/settings/LoggerId.hpp (4)
3-7: LGTM: Clean macro definition with proper default.The include and macro definition are well-structured. The conditional definition allows CMake to override the default while providing a sensible fallback.
19-21: Well-implemented constexpr string comparison.The use of
std::string_viewfor compile-time string comparison is efficient and appropriate. This enables the static_assert validation at compile time.
23-28: Excellent compile-time validation.The static_assert provides robust validation of the verbosity level macro at compile time, preventing invalid configurations from compiling. This addresses potential runtime errors early in the build process.
47-50: Excellent design for compile-time logging optimization.The type aliases create a clean abstraction that enables compile-time log filtering. The Logger alias uses LoggerWrapper with compile-time verbosity evaluation, which should provide the performance benefits mentioned in the PR objectives.
|
| if (!socket.is_open()) { | ||
| return "(N/A, socket is not open)"; | ||
| } | ||
| return socket.remote_endpoint().address().to_string(); |
There was a problem hiding this comment.
Are we sure this will always work and does not omit exceptions?
| INVALID = -1, | ||
| MQTT | ||
| MQTT, | ||
| DUMMY |
There was a problem hiding this comment.
Documentation for each Enum element?
| connectMessage->set_company(company); | ||
| connectMessage->set_vehiclename(vehicleName); | ||
|
|
||
| connectMessage->mutable_devices()->Reserve(devices.size()); |
| return; | ||
| } | ||
| else if (client_ != nullptr) { | ||
| if (client_ != nullptr) { |
There was a problem hiding this comment.
Why was the "else" deleted? Oo, I see if it is connected I want to close the connection in this case.
| return connection->deviceId->isSame(toCompare->deviceId); | ||
| }); | ||
| const auto it = std::find_if(connectedDevices_.begin(), connectedDevices_.end(), | ||
| [&connection](const std::shared_ptr<structures::Connection> &toCompare) { |
https://youtrack.bringauto.com/issue/BAF-1122/Module-Gateway-performance
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests