swap cx destroy metrics

[billcchung]

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: Fixes the reversed metrics count of cx_destroy_remote_with_active_rq and cx_destroy_local_with_active_rq for mongo_proxy
Additional Description:
Risk Level: Low
Testing: tests were already in place but the expectation were also reversed.
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

[repokitteh-read-only]

Hi @billcchung, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #13991 was opened by billcchung.

see: more, trace.

[antoniovicente]

cc @cpakulski

Thanks for noticing that the stats counters seem to be reversed and sending out a PR to correct them.

cpakulski: You seem more familar with this code, could you review the changes once CI is fixed?

[antoniovicente]

This change triggered some continuous integration test failures in test/extensions/filters/network/mongo_proxy/proxy_test.cc due to tests that covered the swapped behavior. I think the solution is to also swap the counter name in the test expectations.

2020-11-12T06:48:59.5799388Z [ RUN ] MongoProxyFilterTest.ConnectionDestroyLocal
2020-11-12T06:48:59.5800234Z TestRandomGenerator running with seed -291122280
2020-11-12T06:48:59.5800928Z test/extensions/filters/network/mongo_proxy/proxy_test.cc:629: Failure
2020-11-12T06:48:59.5801543Z Expected equality of these values:
2020-11-12T06:48:59.5801953Z 1U
2020-11-12T06:48:59.5802274Z Which is: 1
2020-11-12T06:48:59.5802707Z store_.counter("test.cx_destroy_local_with_active_rq").value()
2020-11-12T06:48:59.5803311Z Which is: 0
2020-11-12T06:48:59.5803658Z Stack trace:
2020-11-12T06:48:59.5804212Z 0x6ad607: Envoy::Extensions::NetworkFilters::MongoProxy::MongoProxyFilterTest_ConnectionDestroyLocal_Test::TestBody()
2020-11-12T06:48:59.5805303Z 0x1050e78: testing::internal::HandleExceptionsInMethodIfSupported<>()
2020-11-12T06:48:59.5805925Z 0x1050da5: testing::Test::Run()
2020-11-12T06:48:59.5806456Z 0x1051940: testing::TestInfo::Run()
2020-11-12T06:48:59.5806840Z ... Google Test internal frames ...
2020-11-12T06:48:59.5807102Z
2020-11-12T06:48:59.5807487Z test/extensions/filters/network/mongo_proxy/proxy_test.cc:630: Failure
2020-11-12T06:48:59.5808029Z Expected equality of these values:
2020-11-12T06:48:59.5808351Z 0U
2020-11-12T06:48:59.5808649Z Which is: 0
2020-11-12T06:48:59.5809211Z store_.counter("test.cx_destroy_remote_with_active_rq").value()
2020-11-12T06:48:59.5809606Z Which is: 1
2020-11-12T06:48:59.5809922Z Stack trace:
2020-11-12T06:48:59.5810452Z 0x6ad775: Envoy::Extensions::NetworkFilters::MongoProxy::MongoProxyFilterTest_ConnectionDestroyLocal_Test::TestBody()
2020-11-12T06:48:59.5811100Z 0x1050e78: testing::internal::HandleExceptionsInMethodIfSupported<>()
2020-11-12T06:48:59.5811686Z 0x1050da5: testing::Test::Run()
2020-11-12T06:48:59.5812091Z 0x1051940: testing::TestInfo::Run()
2020-11-12T06:48:59.5812470Z ... Google Test internal frames ...

[cpakulski]

Agree. It seems that counters are reversed. cx_destroy_local_with_active_rq should be incremented for Network::ConnectionEvent::Local close.

As @antoniovicente pointed out, unit tests should be adjusted in https://github.com/envoyproxy/envoy/blob/master/test/extensions/filters/network/mongo_proxy/proxy_test.cc#L629-L630 and https://github.com/envoyproxy/envoy/blob/master/test/extensions/filters/network/mongo_proxy/proxy_test.cc#L653-L654.

Btw, the same logic and counters are present in dubbo proxy, but they are incremented correctly there.

[billcchung]

thanks @antoniovicente @cpakulski , I opened this PR just to remind myself to pick it up later, didnt expect it to be review immediately, but thanks for reviewing & confirming, I've updated the tests as well, CI seems not very stable so I rerun it twice, this can be reviewed I think, thanks!

[antoniovicente]

It would be good to add a release note to the "Minor Behavior Changes" section since this change affects monitoring behavior.

Release notes can be found at: docs/root/version_history/current.rst

[cpakulski]

Agree, please add a release note. Otherwise looks good.