Mongo sink improvements

[sandorzm]

When using the Mongo sink in an application that also uses MongoCXX separately, there's the question of who should own the mongocxx::instance object because exactly one may exist in a program, and its lifetime must enclose the lifetimes of all other MongoCXX objects in the program (MongoCXX docs). Currently, mongo_sink has a static field that owns the instance, causing an exception when any other code in the app tries to construct an instance.

I think it makes more sense for the main application to own it for these reasons:

1. The Mongo sink never does anything with it, but the application might have to access or configure it. From MongoCXX's instance management example:

   real programs will typically require shared access to a commonly configured instance and connection pool across different translation units and components.

   This is impossible if mongo_sink owns the instance.

2. If the app was already using MongoCXX before starting to use Spdlog or mongo_sink, it already has code to construct and manage the MongoCXX instance, and that code would now break because an instance was already constructed by mongo_sink.

I implemented this by making a non-breaking change that makes mongo_sink optionally own the instance, only not owning it if one already exists or if an additional optional argument to the constructor is passed. It's still as convenient to use mongo_sink if not managing a MongoCXX instance separately, but it's also possible to prevent mongo_sink from creating one. Let me know if there's a better way.

A couple other small changes: querying logs by a certain log level or above (e.g., warn, error, critical or debug, warn, error, critical) is a common use case, and this is made much easier by putting the numerical log level in MongoDB along with the string log level. Also removed a G++ warning about catching an exception by value.

[gabime]

I think a prefferable approach would be to have new ctor overload with a shared_ptr param and keep the orig API. something like

mongo_sink(std::shared_ptr<mongocxx::instance> instance, const std::string &db_name, const std::string &collection_name, const std::string &uri): 
   instance_(instance), 
   db_name_(db_name),
   collection_name_(collection_name)
{
    client_ = spdlog::details::make_unique<mongocxx::client>(mongocxx::uri{uri});
}

This is would also ensure that the instance doesn't get destroyed while the sink holds it.
Then the orig ctor could just create the instance and pass it to this new ctor.
Also, the instance_ better not be static now since it is not thread safe to copy same shared_ptr concurrently, and there is no good reason anyway. (to create a singleton instance its better to use a function with static member).

[sandorzm]

Yeah, good point. I split the constructor into two, one creating the instance and delegating to the other. If the creation of the instance throws an exception, I just let that propagate because the constructor that creates the instance should not be called if one already exists.

[gabime]

How about adding a catch clause to catch the mongo exception to extract its what() string and add to the spdlog_ex message as well ?

[sandorzm]

I added the what() string to the spdlog_ex in the std::exception catch clause, which should also catch mongocxx::exception. Did you specifically want a separate catch clause for mongocxx::exception?

Also caught the mongocxx::exception that may be triggered by constructing the mongocxx::instance in the first constructor overload with a function try block, but other std::exceptions are not handled in this overload and would be re-thrown as-is. Is it better to catch all std::exceptions and re-throw them as spdlog_ex?

[gabime]

yes. but since the first ctor just forwards to the other ctor, the try catch is redundant in the first ctor anyway

[sandorzm]

But can't the try-catch in the first constructor catch exceptions from the std::make_shared<mongocxx::instance>() that appears in its initializer list while the second can't?

[sandorzm]

Unless I'm mistaken, I'll change the first constructor's try-catch to catch any std;:exception and re-throw as spdlog_ex.

[gabime]

My mistake, you are right. Merging..

[gabime]

yes. but since the first ctor just forwards to the other ctor, the try catch is redundant in the first ctor anyway

[gabime]

Thanks @sandorzm

[sandorzm]

Thank you for Spdlog, @gabime. I made this PR because my team wants to use the Mongo sink, and we would prefer to incorporate it with this PR's changes in the next tagged release of Spdlog (otherwise, we'll just use a recent commit from the main branch). Do you have a plan for the next release?

[gabime]

There is still work to be done before next release (bump fmt to 9.1.0) so might take a while. Taking the latest commit sounds fine to me anyway.