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

V2.x - Attribute support #3128

Open
wants to merge 15 commits into
base: v2.x-deprecated
Choose a base branch
from

Conversation

felixjulianheitmann
Copy link

This is quite similar to the MDC support by #2907 except for one major difference: MDC is stored in global thread local storage vs. attributes are stored per logger.

Each logger has an attributes_ map which is a std::string/std::string map. During logging, these attributes are passed with the log message to the formatter. An attributes_formatter (%*) will format these the same way that the mdc_formatter does.

I could use inheritance or a common function to reduce the code duplication.

What's the use case?

We are using spdlog on a webserver and have multiple threads handling requests. They share a common logger instance. Since there are multiple threads writing to the same logger, we cannot use MDC to associate additional context to the log messages. The thread writing to the sink will always be a different one than the one executing the web-handler.

This approach will provide the ability to have context associated with a logger instance rather than with a thread. I can have multiple threads writing to the same logger and providing context.

Considerations

1. It's thread safe but is it useful?

After the implementation, I noticed a flaw in my train of thoughts. If I have multiple threads that can safely put context on a logger and write them together with the log messages, I still get a form of race condition.

Logger A wants to do some file operation and calls logger.attrs().put("file-path", "my/path.txt") to have the file path be logged with any subsequent logs.

Logger B may fail parsing an incoming JSON and calls logger.attrs().put("error", exception.what()).

If these two then log "at the same time", the logging message will contain both context attributes.

Maybe I shouldn't reuse the same logger for entirely different operations. But even if they are similar, they are not the same and logs from one task don't matter for the other. I am now quite confident, this is what @tt4g tried to explain to me in #3083 (comment) that I am just grasping now.

2. using a different map

Currently every attribute has to be formatted as a string before inserting it into the API. Maybe this can be worked around by providing a formattable type or similar in the attributes API rather than a string. I haven't dug into fmtlib/std::format enough to see whether that's possible somehow without going down the template rabbit hole.

M4rFri and others added 11 commits July 5, 2024 12:54
The logger interface should not be bloated by additional functionality. Reducing it to a nested call to `.attrs().put()` achieves the same nesting as `.put_attribute()` but requires a smaller interface change on the logger class.
Otherwise the attributes are printed in pseudo-random order.
Previous functions were named poorly due to miscommunication between the devs.

All functions to put attributes on a logger are `put`.
Oftentimes you want to put attributes into a logger (file names, ids, etc.) only for a specific scope (e.g. a function or an if-clause).

Creating a scoped-context will allow this, by returning an RAII-object that will remove the inserted keys upon destruction.
@felixjulianheitmann
Copy link
Author

  1. It's thread safe but is it useful?

Maybe the goal is slightly different. Maybe, I do want thread local storage but on a per-logger basis and I would want my contextual info to be forwarded to the thread pool actually writing to the sinks.

I cannot use single-threaded loggers but I also don't really see the benefit of having multiple threads mix their current individual context

@felixjulianheitmann
Copy link
Author

Okay, some builds seem to fail. I will have a look at that next week.

@felixjulianheitmann
Copy link
Author

Regardless, of whether the additional builds pass, this PR is in a philosophical dilemma.
As far as I can see, there is two ways of implementing contextual attributes for log messages:

  1. an attribute map valid across all logger instances on a per thread basis
    -> current MDC implementation does that
  2. an attribute map valid across all threads on a per logger instane basis
    -> this PR implements that.

Shortcomings of 1.:

  • no async loggers possible
  • unrelated loggers logging on the same thread share the context state

Shortcomings of 2.:

  • contextual info is shared across possibly unrelated tasks. Our use-case of a thread pool for web-handlers sharing a single logger instance would mix the contextual state of possibly different tasks.

A final less convenient solution: per-message context info
We could overload the info/warn/error/... calls to allow the inclusion of per message contextual info. That kind of defeats the purpose though. If I want to include additional info in this specific message ... just write it into the message.

My bottom line is: this PR makes sense to me, personally. For the project I want to use it for, our structure needs to be updated. If we used a separate logger instance per web-handler thread pool, this PR would perfectly solve our issues.

I will go ahead and see, if I can fix the build issues and discuss internally, how to proceed.

@tt4g
Copy link
Contributor

tt4g commented Jul 9, 2024

As far as I know, logging libraries are torn between implementing either of these two.
Personally, I prefer this PR implementation.

@felixjulianheitmann
Copy link
Author

Future improvements could include templated versions that accept any form of attribute type that's formattable by the fmt library. This implementation is kept simple for a first implementations sake.

Any future work on this should be doable without breaking the interface.

constexpr doesn't need to be captured. failed ci/cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants