feat(minimetrics): Add internal recursion protection

[mitsuhiko]

This fixes an issue where metrics can feed back into themselves. It was assumed that this has been correctly prevented as the tests did not surface the internal metrics. However the tests were incorrect as the internal recursion did not go to the test's backend, but the default metrics backend.

For the regular minimetrics backend the fixture was thus changed to patch the global backend:

@pytest.fixture(scope="function")
def backend():
    rv = MiniMetricsMetricsBackend(prefix="sentrytest.")
    with mock.patch("sentry.utils.metrics.backend", new=rv):
        yield rv

I temporarily considered moving metrics_noop to sentry itself, but the protection still makes sense in the main SDK. Reason being that the transport is in parts (via client) invoked from the flush logic and this can create metrics cycles even without the sentry specific metrics system.

Refs INC-528

[mitsuhiko]

Removing any of the two fixes, will now make the tests fail reliably.

[mitsuhiko]

This is the actual fix #1

[mitsuhiko]

This is the actual fix #2.

[mitsuhiko]

Note here: i want to add a change to the python SDK which protects the manual flush call. Once that lands, the real_emit must not be marked as metrics_noop or it stops being called. The unittests should catch this though.

[iambriccardo]

I would honestly fix it in a different way, since this code works under the assumption that the SDK sets internally the thread local. I would honestly implement it differently and more reliably by just adding the @enter_minimetrics decorator to the monkeypatched functions and protecting the call to the SDK inside of the MinimetricsMetricsBackend.

[iambriccardo]

What do you think?

[iambriccardo]

Similar to #57782 but with the finally implementation since it doesn't clear up the state in case of an exception.

[mitsuhiko]

since this code works under the assumption that the SDK sets internally the thread local

I don't believe the code makes such an assumption as metrics_noop sets the thread local.

The risk of a second thread local is this one:

I temporarily considered moving metrics_noop to sentry itself, but the protection still makes sense in the main SDK. Reason being that the transport is in parts (via client) invoked from the flush logic and this can create metrics cycles even without the sentry specific metrics system.

I believe if they do not use the same thread local we have paths where only minimetrics protects itself, but not the outer code path.

[iambriccardo]

Ah ok so it sets also the thread local, I thought it was only checking it and not triggering the function.

[iambriccardo]

After my doubts were answered, LGTM