MINIFICPP-2494 windows event log: fix event message formatting, refactor #1900
Conversation
| @@ -74,7 +74,7 @@ const short event_type_index = 178; // NOLINT short comes from WINDOWS API | |||
|
|
|||
| class FakeWindowsEventLogMetadata : public WindowsEventLogMetadata { | |||
| public: | |||
| [[nodiscard]] std::string getEventData(EVT_FORMAT_MESSAGE_FLAGS flags) const override { return "event_data_for_flag_" + std::to_string(flags); } | |||
| [[nodiscard]] nonstd::expected<std::string, std::error_code> getEventData(EVT_FORMAT_MESSAGE_FLAGS flags) const noexcept override { return "event_data_for_flag_" + std::to_string(flags); } | |||
There was a problem hiding this comment.
Changed this to noexcept. There are allocations here. If they fail, the test will terminate and report failure, which is an acceptable behavior IMO.
| } | ||
| // TODO(szaszm): add error logging |
There was a problem hiding this comment.
One behavior change in getEventData: allocation failure now returns an error_code, instead of an empty string. Since both empty strings and errors are ignored here, there is no change in overall behavior. Added a comment to do proper error handling later, but I didn't wanna do it in the context of this change. (MetadataWalker has no logger.)
| std::map<std::string, std::string> MetadataWalker::getFieldValues() const { | ||
| return fields_values_; | ||
| } | ||
|
|
||
| std::map<std::string, std::string> MetadataWalker::getIdentifiers() const { | ||
| return replaced_identifiers_; |
There was a problem hiding this comment.
moved inline to the header
| case USER: // TODO(szaszm): unhandled before refactoring | ||
| case UNKNOWN: // TODO(szaszm): unhandled before refactoring |
There was a problem hiding this comment.
I'm not sure how to properly handle these, so I opted to not change the behavior.
| EVT_HANDLE WindowsEventLogHandler::getMetadata() const { | ||
| return metadata_provider_.get(); | ||
| } |
There was a problem hiding this comment.
moved inline to the header
| static std::string getMetadataString(METADATA val) { | ||
| static std::map<METADATA, std::string> map = { |
| } else { | ||
| return METADATA::UNKNOWN; | ||
| } | ||
| static Metadata getMetadataFromString(std::string_view val) { |
There was a problem hiding this comment.
Changed this to be thread-safe, and avoid a static map, at the expense of linear lookup instead of logarithmic. I think it doesn't matter at such a small and constant size.
| static std::string computer_name; | ||
| if (computer_name.empty()) { | ||
| char buff[10248]; | ||
| static std::string computer_name = []() -> std::string { |
There was a problem hiding this comment.
Moved this to an immediately invoked lambda: this way the thread-safe static initialization takes care of only running the initialization once, and subsequent calls will just copy the static string. The old version was not thread-safe.
| for (const auto& option : header_names_) { | ||
| auto name = option.second; | ||
| const auto& name = option.second; |
There was a problem hiding this comment.
option.second is a std::string
| @@ -296,7 +296,7 @@ int64_t OsUtils::getTotalPagingFileSize() { | |||
| return total_paging_file_size; | |||
| } | |||
|
|
|||
| std::error_code OsUtils::windowsErrorToErrorCode(DWORD error_code) { | |||
| std::error_code OsUtils::windowsErrorToErrorCode(DWORD error_code) noexcept { | |||
There was a problem hiding this comment.
No allocations happen here, so it might as well be noexcept
…tors Signed-off-by: Marton Szasz <[email protected]>
szaszm
force-pushed
the
MINIFICPP-2494
branch
from
c6436e7 to
2964484
Compare
| // this is a continuous enum, so we can rely on the array | ||
|
|
||
| using METADATA_NAMES = std::vector<std::pair<METADATA, std::string>>; | ||
| nonstd::expected<std::string, std::error_code> formatEvent(EVT_HANDLE metadata, EVT_HANDLE event, EVT_FORMAT_MESSAGE_FLAGS flags) noexcept; |
There was a problem hiding this comment.
EVT_HANDLE is void*, but I couldn't wrap it in gsl::not_null, because it's incompatible with void*. (operator* cannot return a reference to void)
|
I wouldn't call this premature optimization, this rewrite had a measurable 20% throughput increase: #928 (comment) |
|
my bad then. Back to draft, and I'll wait for confirmation to see if this fixes the crash. |
Changes:
enum METADATAtoenum class MetadatagetEventDataandgetEventMessage. They no longer try with a stack buffer first. That looks like a premature optimization, since there are allocations in the function either way, and a user reported issues with the call that may be resolved by this. The rewrite aims to follow modern C++ best practices, instead of the unsafe C-like code it used to be.getComputerNamethread-safe and simpler. The max computer name is reduced to 256, based on the MSDN example. I think the overwhelming majority of computer FQDNs will fit in that just fine. Also changed it to truncate when it's longer, instead of bailing out and returning "N/A"Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.