Use official MongoDB C Driver instead of libmongo-client

[bkil-syslogng]

Rebased and migrated #728
Should be almost the be the same codebase, except:

• added a missing g_free(uri)
• reordered two variables
• deleted an extra lines

After somebody does sanity checking to verify that the rebase went alright and they like the squashed commit organization, it would be ready for merge from my perspective.

Please assign it to someone else if you are assigned and you don't have time for this.

[bkil-syslogng]

I have fixed the warnings that made it fail, so now I am ready.

[ihrwein]

👍

[ihrwein]

Configuration changes: servers, host, port, path, database, username, password, safe-mode are removed. uri is added, which accepts a mongodb uri as its parameter (https://docs.mongodb.org/manual/reference/connection-string/).

These changes are effective on the master branch and will be published first in syslog-ng 3.8. @fekete-robert may be also interested in this.

[fekete-robert]

Thanks, I've added it to our todo list.
What will happen to the old parameters? Deprecated in 3.8 and removed in
3.9, or?

On Mon, Feb 1, 2016 at 10:40 AM, Tibor Benke notifications@github.com
wrote:

Configuration changes: servers, host, port, path, database, username,
password, safe-mode are removed. uri is added, which accepts a mongodb
uri as its parameter (
https://docs.mongodb.org/manual/reference/connection-string/).

These changes are effective on the master branch and will be published
first in syslog-ng 3.8. @fekete-robert https://github.com/fekete-robert
may be also interested in this.

—
Reply to this email directly or view it on GitHub
#891 (comment).

[ihrwein]

They are completely removed (without deprecation).

[bazsi]

Well, that's not nice. This breaks user configuration, which we should nake
an effort not to do.

Is it impossible to support those options?
On Feb 1, 2016 12:35 PM, "Tibor Benke" notifications@github.com wrote:

They are completely removed.

—
Reply to this email directly or view it on GitHub
#891 (comment).

[ihrwein]

The complete underlying library was swapped out. The new lib supports these options, but they are embedded into an URI. We may add a layer (construct the URI based on these options...), but I don't think it's worth the effort. We would eventually do the complete removal.

[bazsi]

Well, how difficult would it be to do that layer. We tend to be upward
compazible (just like the kernel) and it is an important policy.
On Feb 1, 2016 1:02 PM, "Tibor Benke" notifications@github.com wrote:

The complete underlying library was swapped out. The new lib supports
these options, but they are embedded into an URI. We may add a layer
(construct the URI based on these options...), but I don't think it's worth
the effort. We would eventually do the complete removal.

—
Reply to this email directly or view it on GitHub
#891 (comment).

[ihrwein]

It's not difficult, but IMHO error prone. What if we warn the user about the new simplified config if he/she uses one of the old configuration parameters? I know compatibility is very important, but if we always stick to it we can never get rid of some not-so-good decisions. Also, 3.8 is a major release.

[bazsi]

Telling the user to migrate to the new options is fine and is the policy we applied in the past. We can even remove options if a long enough grace period is available.

The fact that 3.8 is a major release is not an excuse to break user configurations, our intent is to make upgrades (even accross major versions) as seamless as possible.

Breaking configurations this way would upset users of that feature and rightly so.

I would consider this a critical bug.

[bkil-syslogng]

We discussed this with Laci and those interested and concluded that
retaining backwards compatibility was not feasible. Actually the first
incarnation before the refactor did reconstruct some (most?) of the
original options into the URL. I think I still have such a before-rebase
branch in my backups.

However, it could not be guaranteed that the layered functionality would
remain 100% the same as when using the other connection sublayer.
Especially that some of the options were/are proprietary to one or the
other and it wouldn't make sense to even emulate them. In addition, I would
definitely not want to take in bug reports to the effect that semantics
changed and introduce some more kludges to support such a temporary extra
layer. The present solution is also very clean and small, and thus much
more reliable in the end.

I recall that we have also asked Ricsi about who asked for this
functionality originally and how many current users we have, and we seem to
have agreed that backwards compatibility isn't that important in this case.
Please refute me if I recall incorrectly.

All in all, my vote is that no compatibility layer will be added and this
will stay in as is.

On Mon, Feb 1, 2016 at 8:06 PM, Balazs Scheidler notifications@github.com
wrote:

Telling the user to migrate to the new options is fine and is the policy
we applied in the past. We can even remove options if a long enough grace
period is available.

The fact that 3.8 is a major release is not an excuse to break user
configurations, our intent is to make upgrades (even accross major
versions) as seamless as possible.

Breaking configurations this way would upset users of that feature and
rightly so.

I would consider this a critical bug.

—
Reply to this email directly or view it on GitHub
#891 (comment).

[bazsi]

Sorry, but this is still not convincing enough. In the past 18 years,
upward compatibility was considered important, and although there were
times where unintentionally were breaking it, but this was a mistake even
at those times.

We do have infrastructure in place to "upgrade" options to a saner
structure, but we have to make an effort to keep functionality working even
with old configuration versions. We can make tradeoffs there (even ignoring
various options), but producing a "syntax error" is a very unfriendly way
to convince the customer to change his/her configuration.

I might be missing something related to why it is so difficult to implement
the compatibility layer, but I'd love to understand before letting this go.

We don't have a clue how many people use the mongodb functionality, but if
it's greater than zero, we are still pissing of someone. Not to mention
example blog posts around that will have incorrect information continuing
even to the future.

Bazsi

On Wed, Feb 3, 2016 at 9:20 AM, Tamas Nagy notifications@github.com wrote:

We discussed this with Laci and those interested and concluded that
retaining backwards compatibility was not feasible. Actually the first
incarnation before the refactor did reconstruct some (most?) of the
original options into the URL. I think I still have such a before-rebase
branch in my backups.

However, it could not be guaranteed that the layered functionality would
remain 100% the same as when using the other connection sublayer.
Especially that some of the options were/are proprietary to one or the
other and it wouldn't make sense to even emulate them. In addition, I would
definitely not want to take in bug reports to the effect that semantics
changed and introduce some more kludges to support such a temporary extra
layer. The present solution is also very clean and small, and thus much
more reliable in the end.

I recall that we have also asked Ricsi about who asked for this
functionality originally and how many current users we have, and we seem to
have agreed that backwards compatibility isn't that important in this case.
Please refute me if I recall incorrectly.

All in all, my vote is that no compatibility layer will be added and this
will stay in as is.

On Mon, Feb 1, 2016 at 8:06 PM, Balazs Scheidler <notifications@github.com

wrote:

Telling the user to migrate to the new options is fine and is the policy
we applied in the past. We can even remove options if a long enough grace
period is available.

The fact that 3.8 is a major release is not an excuse to break user
configurations, our intent is to make upgrades (even accross major
versions) as seamless as possible.

Breaking configurations this way would upset users of that feature and
rightly so.

I would consider this a critical bug.

—
Reply to this email directly or view it on GitHub
#891 (comment).

—
Reply to this email directly or view it on GitHub
#891 (comment).

[bkil-syslogng]

Actually, I'm sad and surprised that you only now share this completely
opposing viewpoint. The changeset has been subject to a very lax 4 MONTH
review process. This said breaking change was isolated to a single commit
on top of all the others and was easily reviewable for even those who did
not wish to go through the whole thing. Actually now that I open it, the
said change structure could be found in the previous version linked here as
well, so it's not only present in my backups.

See:

#728 (comment)
#728 (comment)
u2yg@f6cfb2c

Putting the handling of this process itself aside, I'm still not convinced
that we should keep compatibility. However, if you can convince the whole
team that it is needed, a kind of compatibility layer could be bolted on
top of this one similar to how it was done before the last commit. We're
opening a whole can of worms and need some more sanity checking when
supporting both the URI and legacy fields at the same time, though. Do note
that I can not guarantee any kind of compatibility unless we ship both
mongo-c-driver and libmongo-client at the same time. With this in mind, we
could just as well create a separate afmongodburi destination to
encapsulate this different mechanism while we are at it, but I feel that we
are pushing it too far. What if we provided a syslog-ng.conf migration
assistant to advise a user in case of such incompatibilities?

Until which version would you support the old mongodb config syntax?

On Wed, Feb 3, 2016 at 12:02 PM, Balazs Scheidler notifications@github.com
wrote:

Sorry, but this is still not convincing enough. In the past 18 years,
upward compatibility was considered important, and although there were
times where unintentionally were breaking it, but this was a mistake even
at those times.

We do have infrastructure in place to "upgrade" options to a saner
structure, but we have to make an effort to keep functionality working even
with old configuration versions. We can make tradeoffs there (even ignoring
various options), but producing a "syntax error" is a very unfriendly way
to convince the customer to change his/her configuration.

I might be missing something related to why it is so difficult to implement
the compatibility layer, but I'd love to understand before letting this go.

We don't have a clue how many people use the mongodb functionality, but if
it's greater than zero, we are still pissing of someone. Not to mention
example blog posts around that will have incorrect information continuing
even to the future.

Bazsi

On Wed, Feb 3, 2016 at 9:20 AM, Tamas Nagy notifications@github.com
wrote:

We discussed this with Laci and those interested and concluded that
retaining backwards compatibility was not feasible. Actually the first
incarnation before the refactor did reconstruct some (most?) of the
original options into the URL. I think I still have such a before-rebase
branch in my backups.

However, it could not be guaranteed that the layered functionality would
remain 100% the same as when using the other connection sublayer.
Especially that some of the options were/are proprietary to one or the
other and it wouldn't make sense to even emulate them. In addition, I
would
definitely not want to take in bug reports to the effect that semantics
changed and introduce some more kludges to support such a temporary extra
layer. The present solution is also very clean and small, and thus much
more reliable in the end.

I recall that we have also asked Ricsi about who asked for this
functionality originally and how many current users we have, and we seem
to
have agreed that backwards compatibility isn't that important in this
case.
Please refute me if I recall incorrectly.

All in all, my vote is that no compatibility layer will be added and this
will stay in as is.

On Mon, Feb 1, 2016 at 8:06 PM, Balazs Scheidler <
notifications@github.com

wrote:

Telling the user to migrate to the new options is fine and is the
policy
we applied in the past. We can even remove options if a long enough
grace
period is available.

The fact that 3.8 is a major release is not an excuse to break user
configurations, our intent is to make upgrades (even accross major
versions) as seamless as possible.

Breaking configurations this way would upset users of that feature and
rightly so.

I would consider this a critical bug.

—
Reply to this email directly or view it on GitHub
<#891 (comment)
.

—
Reply to this email directly or view it on GitHub
#891 (comment).

—
Reply to this email directly or view it on GitHub
#891 (comment).

[bazsi]

I for one don't review all patches and I never intended to (that would be a
bottleneck), but breaking changes like this should be communicated much
better (e.g. right in the text of the pull request).

That said I think this question is not an issue that should be decided by
engineers alone, this kind of thing would have to be decided by the
product, its vision and strategy. Configuration file is a user interface,
just like command line options for sed or grep. What would you think if sed
dropped an option when they change the regexp engine from the libc one to
pcre? I guess once you rely on it in your shell scripts, you wouldn't like
the idea.

A seemingly internal change (e.g. change the implementation of a
communication library) should not cause user-visible changes, period. But
like you said, let's put the communication issue aside.

I think there are numerous trade-offs and solutions that can be made at
this point, some requiring more effort others less so. Some being more
fragile, others less so. I didn't say that I would like a fragile,
unmaintainable mess.

But I wanted to draw a clear line in the sand: breaking the user
configuration (especially hard-to-troubleshoot ways) is not a good approach
and we have to think hard how we will guide the user to the new version.

Look at for example the systemd workarounds in the unix-stream() driver,
those are not nice either but was required in order to keep syslog-ng
running as users were upgrading their systems.

How far we would want to keep compatibility I don't know, it is a tradeoff
that also depends on the kind of solution that we create. It should be
supported at least until everyone has definitely migrated off from versions
that used the old syntax. So far we could get away from complete
architectural revamps of syslog-ng keeping the compatibility with old
versions, this should be possible in most cases.

Bazsi

On Wed, Feb 3, 2016 at 1:55 PM, Tamas Nagy notifications@github.com wrote:

Actually, I'm sad and surprised that you only now share this completely
opposing viewpoint. The changeset has been subject to a very lax 4 MONTH
review process. This said breaking change was isolated to a single commit
on top of all the others and was easily reviewable for even those who did
not wish to go through the whole thing. Actually now that I open it, the
said change structure could be found in the previous version linked here as
well, so it's not only present in my backups.

See:

#728 (comment)
#728 (comment)

u2yg@f6cfb2c

Putting the handling of this process itself aside, I'm still not convinced
that we should keep compatibility. However, if you can convince the whole
team that it is needed, a kind of compatibility layer could be bolted on
top of this one similar to how it was done before the last commit. We're
opening a whole can of worms and need some more sanity checking when
supporting both the URI and legacy fields at the same time, though. Do note
that I can not guarantee any kind of compatibility unless we ship both
mongo-c-driver and libmongo-client at the same time. With this in mind, we
could just as well create a separate afmongodburi destination to
encapsulate this different mechanism while we are at it, but I feel that we
are pushing it too far. What if we provided a syslog-ng.conf migration
assistant to advise a user in case of such incompatibilities?

Until which version would you support the old mongodb config syntax?

On Wed, Feb 3, 2016 at 12:02 PM, Balazs Scheidler <
notifications@github.com>

wrote:

Sorry, but this is still not convincing enough. In the past 18 years,
upward compatibility was considered important, and although there were
times where unintentionally were breaking it, but this was a mistake even
at those times.

We do have infrastructure in place to "upgrade" options to a saner
structure, but we have to make an effort to keep functionality working
even
with old configuration versions. We can make tradeoffs there (even
ignoring
various options), but producing a "syntax error" is a very unfriendly way
to convince the customer to change his/her configuration.

I might be missing something related to why it is so difficult to
implement
the compatibility layer, but I'd love to understand before letting this
go.

We don't have a clue how many people use the mongodb functionality, but
if
it's greater than zero, we are still pissing of someone. Not to mention
example blog posts around that will have incorrect information continuing
even to the future.

Bazsi

On Wed, Feb 3, 2016 at 9:20 AM, Tamas Nagy notifications@github.com
wrote:

We discussed this with Laci and those interested and concluded that
retaining backwards compatibility was not feasible. Actually the first
incarnation before the refactor did reconstruct some (most?) of the
original options into the URL. I think I still have such a
before-rebase
branch in my backups.

However, it could not be guaranteed that the layered functionality
would
remain 100% the same as when using the other connection sublayer.
Especially that some of the options were/are proprietary to one or the
other and it wouldn't make sense to even emulate them. In addition, I
would
definitely not want to take in bug reports to the effect that semantics
changed and introduce some more kludges to support such a temporary
extra
layer. The present solution is also very clean and small, and thus much
more reliable in the end.

I recall that we have also asked Ricsi about who asked for this
functionality originally and how many current users we have, and we
seem
to
have agreed that backwards compatibility isn't that important in this
case.
Please refute me if I recall incorrectly.

All in all, my vote is that no compatibility layer will be added and
this
will stay in as is.

On Mon, Feb 1, 2016 at 8:06 PM, Balazs Scheidler <
notifications@github.com

wrote:

Telling the user to migrate to the new options is fine and is the
policy
we applied in the past. We can even remove options if a long enough
grace
period is available.

The fact that 3.8 is a major release is not an excuse to break user
configurations, our intent is to make upgrades (even accross major
versions) as seamless as possible.

Breaking configurations this way would upset users of that feature
and
rightly so.

I would consider this a critical bug.

—
Reply to this email directly or view it on GitHub
<
#891 (comment)
.

—
Reply to this email directly or view it on GitHub
<#891 (comment)
.

—
Reply to this email directly or view it on GitHub
#891 (comment).

—
Reply to this email directly or view it on GitHub
#891 (comment).

[bkil-syslogng]

By the way, the PR started as one which does not break the config, and only
then progress onto the saner path. From the present diff and an executive
perspective, you may be right in that the PR description should have
mentioned something like "Move from libmongo-client to mongo-c-driver.
Connect and pass options through a MongoDB URI".

From a retrospective, we have assumed that all of those interested were up
to date on this issue, however now we found out that some of us have
desynced. I'm not sure how we could have protect against such a
misunderstanding, but it would be useful to figure something out for the
future.

On Wed, Feb 3, 2016 at 3:05 PM, Nagy, Tamás tamas.nagy@balabit.com wrote:

On Wed, Feb 3, 2016 at 2:12 PM, Balazs Scheidler <notifications@github.com

wrote:

I for one don't review all patches and I never intended to (that would be
a
bottleneck), but breaking changes like this should be communicated much
better (e.g. right in the text of the pull request).

That's completely understandable and I think I can speak from the heart
of the whole team by saying that we wish you had much less micro management
to do.

Sorry that my (our) communication might not have been clear enough in the
past, but I hope we have improved in the past months. Do note that I did
mention this breaking change, though in a bit awkward fashion in the two
linked comments. There existed multiple (sub)tasks and description fields
in our internal issue tracking system as well that provided an overview
about the kind of changes warranted (including refactoring the higher level
tests because of the config changes), and we have probably mentioned this
on at least a dozen daily standups, so it was anything but
under-communicated. As the whole team was always up to date on this, we can
consider this to be a team decision.

The linked extra commit that I did ask people to review after being added,
contained the following description (which might not have had the best
wording, but it did communicate intent):

afmongodb: uri option supersedes all, coll efficiency improvement
Added URI option to config file.
Removed all other options from the config file.
Removed unused namespace variable.
Moved mongoc_client_get_collection from ...insert() to ...connect().

After rebasing, most of these changes turned up in the commit with the
following description:

afmongodb: added mongo uri option, removed all redundant ones

Added Mongo connection URI option to config file.
Removed all other options from the config file that can be
passed through the URI.

I did ask that somebody does sanity checking to verify that the rebase went alright and they like the squashed commit organization,, and hence if
somebody thought that the commit messages and the PR description or
anything else was afoul, somebody would again have a second opportunity to
intervene or at least ask for clarification. (But of course, because
everything went as we agreed, no intervention was needed.)

That said I think this question is not an issue that should be decided by

engineers alone, this kind of thing would have to be decided by the
product, its vision and strategy. Configuration file is a user interface,
just like command line options for sed or grep. What would you think if
sed
dropped an option when they change the regexp engine from the libc one to
pcre? I guess once you rely on it in your shell scripts, you wouldn't like
the idea.

A seemingly internal change (e.g. change the implementation of a
communication library) should not cause user-visible changes, period. But
like you said, let's put the communication issue aside.

I think there are numerous trade-offs and solutions that can be made at
this point, some requiring more effort others less so. Some being more
fragile, others less so. I didn't say that I would like a fragile,
unmaintainable mess.

But I wanted to draw a clear line in the sand: breaking the user
configuration (especially hard-to-troubleshoot ways) is not a good
approach
and we have to think hard how we will guide the user to the new version.

Look at for example the systemd workarounds in the unix-stream() driver,
those are not nice either but was required in order to keep syslog-ng
running as users were upgrading their systems.

We agreed that it was a question too difficult to answer by engineer.
That is why we called in the PO from the product side just before making
this decision. I do not pretend that breaking config compatibility was his
decision. However, we have explicitly asked him to go around in house to
ask who asked for this feature in the first place and whether we know about
any user or even heard of such. He returned with negative results, and thus
left it with us to decide. With this, I do not want to highlight that this
was a good decision, but it does prove that it was an informed one and we
did indeed query input from the product side before making it.

I guess most other parts of the world connects using a MongoDB URI in
their systems anyway, and these alternative conventions were fabricated by
us and imposed on us by the weird connector, but correct me if I'm wrong on
this one.

How far we would want to keep compatibility I don't know, it is a tradeoff
that also depends on the kind of solution that we create. It should be
supported at least until everyone has definitely migrated off from
versions
that used the old syntax. So far we could get away from complete
architectural revamps of syslog-ng keeping the compatibility with old
versions, this should be possible in most cases.

Maybe my memory fails me on this one, but I recall that we did consider
this and agreed that in case a former deployment did choose MongoDB in the
past and an upgrade breaks it, we will only fix it then. "Don't fix it if
it ain't broken," they say. You may call this akin to sticking our heads
into the sand, however in the end, we've decided to handle this question
according to its priority: nice to have.

On Wed, Feb 3, 2016 at 1:55 PM, Tamas Nagy notifications@github.com
wrote:

Actually, I'm sad and surprised that you only now share this completely
opposing viewpoint. The changeset has been subject to a very lax 4 MONTH
review process. This said breaking change was isolated to a single
commit
on top of all the others and was easily reviewable for even those who
did
not wish to go through the whole thing. Actually now that I open it, the
said change structure could be found in the previous version linked
here as
well, so it's not only present in my backups.

See:

#728 (comment)
#728 (comment)

u2yg@f6cfb2c

Putting the handling of this process itself aside, I'm still not
convinced
that we should keep compatibility. However, if you can convince the
whole
team that it is needed, a kind of compatibility layer could be bolted on
top of this one similar to how it was done before the last commit. We're
opening a whole can of worms and need some more sanity checking when
supporting both the URI and legacy fields at the same time, though. Do
note
that I can not guarantee any kind of compatibility unless we ship both
mongo-c-driver and libmongo-client at the same time. With this in mind,
we
could just as well create a separate afmongodburi destination to
encapsulate this different mechanism while we are at it, but I feel
that we
are pushing it too far. What if we provided a syslog-ng.conf migration
assistant to advise a user in case of such incompatibilities?

Until which version would you support the old mongodb config syntax?

On Wed, Feb 3, 2016 at 12:02 PM, Balazs Scheidler <
notifications@github.com>

wrote:

Sorry, but this is still not convincing enough. In the past 18 years,
upward compatibility was considered important, and although there were
times where unintentionally were breaking it, but this was a mistake
even
at those times.

We do have infrastructure in place to "upgrade" options to a saner
structure, but we have to make an effort to keep functionality working
even
with old configuration versions. We can make tradeoffs there (even
ignoring
various options), but producing a "syntax error" is a very unfriendly
way
to convince the customer to change his/her configuration.

I might be missing something related to why it is so difficult to
implement
the compatibility layer, but I'd love to understand before letting
this
go.

We don't have a clue how many people use the mongodb functionality,
but
if
it's greater than zero, we are still pissing of someone. Not to
mention
example blog posts around that will have incorrect information
continuing
even to the future.

Bazsi

On Wed, Feb 3, 2016 at 9:20 AM, Tamas Nagy notifications@github.com
wrote:

We discussed this with Laci and those interested and concluded that
retaining backwards compatibility was not feasible. Actually the
first
incarnation before the refactor did reconstruct some (most?) of the
original options into the URL. I think I still have such a
before-rebase
branch in my backups.

However, it could not be guaranteed that the layered functionality
would
remain 100% the same as when using the other connection sublayer.
Especially that some of the options were/are proprietary to one or
the
other and it wouldn't make sense to even emulate them. In addition,
I
would
definitely not want to take in bug reports to the effect that
semantics
changed and introduce some more kludges to support such a temporary
extra
layer. The present solution is also very clean and small, and thus
much
more reliable in the end.

I recall that we have also asked Ricsi about who asked for this
functionality originally and how many current users we have, and we
seem
to
have agreed that backwards compatibility isn't that important in
this
case.
Please refute me if I recall incorrectly.

All in all, my vote is that no compatibility layer will be added and
this
will stay in as is.

On Mon, Feb 1, 2016 at 8:06 PM, Balazs Scheidler <
notifications@github.com

wrote:

Telling the user to migrate to the new options is fine and is the
policy
we applied in the past. We can even remove options if a long
enough
grace
period is available.

The fact that 3.8 is a major release is not an excuse to break
user
configurations, our intent is to make upgrades (even accross major
versions) as seamless as possible.

Breaking configurations this way would upset users of that feature
and
rightly so.

I would consider this a critical bug.

—
Reply to this email directly or view it on GitHub
<
#891 (comment)
.

—
Reply to this email directly or view it on GitHub
<
#891 (comment)
.

—
Reply to this email directly or view it on GitHub
<#891 (comment)
.

—
Reply to this email directly or view it on GitHub
#891 (comment).

—
Reply to this email directly or view it on GitHub
#891 (comment).