Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Custom Fields in quill::MacroMetadata (for filtering) #349

Closed
selevit opened this issue Sep 8, 2023 · 8 comments · Fixed by #375
Closed

Support Custom Fields in quill::MacroMetadata (for filtering) #349

selevit opened this issue Sep 8, 2023 · 8 comments · Fixed by #375
Labels
enhancement New feature or request

Comments

@selevit
Copy link

selevit commented Sep 8, 2023

First of all, thanks for this great library!

If there are any plans to allow passing custom fields into the log metadata? Would be very useful for filtering.

@odygrd
Copy link
Owner

odygrd commented Nov 1, 2023

hey, how are you planning to use it ?

Set some key as part of the log statement which you later use in backend thread to filter it ? Would help if you provide me an example

I believe it is possible to do, my only concern is that it will further complicate the code, meaning we will have to define new LOG_ macros for this and we already have too many

Could something similar be achieved by using a different Logger instance for each field you want to filter ?

@selevit
Copy link
Author

selevit commented Nov 22, 2023

hey, how are you planning to use it ? Set some key as part of the log statement which you later use in backend thread to filter it ?

Hey :)

Yes, to filter it, or to use in a custom formatter to include extra fields into the formatted message. Now I have to use quite a dirty macro hack to do some partial formatting in the hotpath.

Formatter example:

fmt_buffer_t const& CustomFormatter::format(
  fmt_buffer_t const&        formatted_log_message,
  quill::TransitEvent const& log_event)
{
    const auto metadata = log_event.metadata();
    const MyCustomFieldsStruct* custom_fields = metadata.get_custom_fields<MyCustomFieldsStruct>();
      m_message_buf.append(std::format("{} {} {}", custom_fields->field1,  custom_fields->field2, custom_fields->field3, ...)
}

Filter example:

class CustomFilter : public quill::FilterBase
{
public:
  CustomFilter() : quill::FilterBase("CustomFilter"){};

  QUILL_NODISCARD bool filter(char const* thread_id, std::chrono::nanoseconds log_record_timestamp,
                              quill::detail::LogRecordMetadata const& metadata,
                              fmt::memory_buffer const& formatted_record) noexcept override
  {
    const MyCustomFieldsStruct* custom_fields = metadata.get_custom_fields<MyCustomFieldsStruct>();
    if (custom_fields->field1 == "foo")
        return false;
    return true;
  }
};

I believe it is possible to do, my only concern is that it will further complicate the code, meaning we will have to define new LOG_ macros for this and we already have too many

I definitely agree that this functionality can complicate the library's code.

Could something similar be achieved by using a different Logger instance for each field you want to filter?

This approach will work only for filters, not for formatters. But even with filtering using several loggers, the user code will also become complicated, so you need to properly dispatch the correct logger depending on a context, and this is not always possible to do in general, especially when filter rules have a conditional filter logic, e.g. to follow one filter rule if a particular field is set, and another filter rule if this field is not set or have a different value.

Also, to filter out some log events we anyway have to somehow obtain structured custom fields on the Filter side, even if it's per-logger.

In my humble opinion, if there was such a custom fields functionality in Quill, it would improve the library/s user experience so much, and would also cover most of the existing logging cases :)

@odygrd odygrd linked a pull request Nov 24, 2023 that will close this issue
@odygrd
Copy link
Owner

odygrd commented Nov 24, 2023

hey, I added support for custom tags in the above PR, have a look at the example (examples/example_custom_tags.cpp) there and let me know if it works for you.
Since we are adding them to MacroMetadata they are compile-only tags
If we need dynamic tags they have to be added in a different place

@selevit
Copy link
Author

selevit commented Nov 24, 2023

Hey, I took a look at the example and it looks great! Compile-only tags are more than enough.

The only thing that IMHO would be nice to have (but not critical) - to be able to pass several custom tag objects into the macro, in case if it's needed to put them into different places in the message format.

Or to make it somehow possible to add format modifiers for each tag, so we can put specific CustomTags members in the message format string. For example:

 stdout_handler->set_pattern(
    "%(ascii_time) [%(thread)] %(fileline:<28) %(level_name) %(logger_name:<16) - [%(custom_tags.tag_a)] : [%(custom_tags.tag_b]"
    "%(message)",              // format
    "%Y-%m-%d %H:%M:%S.%Qms",  // timestamp format
    quill::Timezone::GmtTime); // timestamp's timezone

But maybe it can be also implemented in user code by a custom formatter...

Thanks for implementing this, i'll try it out in my project once the new release is available in Conan :)

@odygrd
Copy link
Owner

odygrd commented Nov 24, 2023

Hey,

It might prove challenging to support variadic custom tags alongside #VA_ARGS for format arguments within the same macro.

We could store std::vector<CustomFields*>. However, only very recent compilers support constexpr vector.

Using a std::array<CustomTags const*, MAX_TAGS> could simplify storage, but requiring users to create and pass arrays to macros might overly complicate the API.

Instead, my recommendation is to introduce a new user-defined type for each CustomTags combination.

For this purpose, I've included a helper class called CombinedCustomTags.

Please refer to the updated example here.

Addressing the PatternFormatter, supporting it could be complex due to the user-defined nature of the CustomTags class.
The %(custom_tags) placeholder will print whatever formatting the user implements in void format(std::string& out) const override { ... } which must be provided when deriving from quill::CustomTags. Placing each CustomTag in different location of the message I think it will be too complex to implement and I don't see it as too important feature.

As you mentioned for any more advanced functionality with a custom Handler, both below steps are required :

  1. Exclude %(custom_tags) from the PatternFormatter pattern, resulting in formatted_log_message without tag printing.
  2. Develop a custom Handler class to control or print the CustomTags in void write(quill::fmt_buffer_t const& formatted_log_message, quill::TransitEvent const& log_event) override which must be provided when deriving from quill::Handler

@selevit
Copy link
Author

selevit commented Nov 25, 2023

Thanks a lot!

BTW , Is it possible to override the format method for CombinedCustomTags?

How would it be formatted by default? I mean, combined tags related to each other in the resulting string.

@odygrd
Copy link
Owner

odygrd commented Nov 25, 2023

it is only a few lines of code (https://github.com/odygrd/quill/blob/custom_tags/quill/include/quill/Utility.h#L64) you can override format() or just create your own CombinedCustomTags class instead of using the provided one

@odygrd odygrd added the enhancement New feature or request label Nov 25, 2023
@odygrd
Copy link
Owner

odygrd commented Nov 27, 2023

That should be in release v3.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants