Issues with event counters

[roji]

The following counters don't get updated when the async API is used (as opposed to the sync one):

• SaveChanges (flagged in Calls to SaveChangesAsync do not update the EntityFrameworkEventSource values #23604)
• Optimistic concurrency failures
• Execution strategy operation failure

In addition, the "Active DbContexts" counter is incorrect, since it's incremented only when the context's InternalServiceProvider is accessed, but decremented when disposing (regardless of whether InternalServiceProvider was accessed). Am moving the increment to occur in the constructor, /cc @ajcvickers

Note: I've spent some considerable time preparing automated tests for event counters, however:

• Due to the nature of event counters (captured out of process over networking), they seem like they will may be the source of a lot of flakiness and headaches.
• They're really slow (counters get emitted at most once per second).
• Need to be totally isolated from any other EF Core test activity, as the counters are application-global.
• The value we get from preventing regressions doesn't seem very high.
• FWIW neither ASP.NET nor runtime have tests for their event counters.

So am dropping this for now, but we can rediscuss if we feel we need to.

[ajcvickers]

@roji As I understand it, all we do in EF for event counters is to increment a value in EntityFrameworkEventSource. Leaving aside for the moment that there is a shared static instance of this, would it not be possible to write tests where, for example, the code calls SaveChanges 10 times and then we check that the value has been incremented 10 times? This should not involve trying to actually use event counters in the test, but would still have caught these issues had the tests been written.

Coming back to the static/global issue, it seems to me that some refactoring and changing of private code to pubternal could allow a test to be setup with an isolated EntityFrameworkEventSource that could be used in tests.

This doesn't tell us that event counters are working end-to-end, but as you say others are not testing that either because of the issues you outline. However, it would give us a pattern for testing new event counters that verify at least that the "EF part" is working as expected.

[roji]

Leaving aside for the moment that there is a shared static instance of this, would it not be possible to write tests where, for example, the code calls SaveChanges 10 times and then we check that the value has been incremented 10 times?

We could do this, but these counters aren't exposed publicly (at least not at the moment) - the only way to observe them right now is via the perf counters. If you think we should expose them, let me know and I'll do it (that would be introducing public surface in a patch).

Coming back to the static/global issue, it seems to me that some refactoring and changing of private code to pubternal could allow a test to be setup with an isolated EntityFrameworkEventSource that could be used in tests.

I'm not sure the static/global issue is actually blocking testing - xunit does allow turning off parallelization for a specific test collection (with [CollectionDefinition(DisableParallelization = true)]), this should allow us to just test directly on the static/global instance. Other than that, I'm not sure exactly what refactoring you have in mind, but I wanted to avoid doing virtual dispatch etc. in the product just for the sake of testing etc.

Basically I'm fine with anything here (especially after seeing the errors I missed) - let me know what you prefer me to do. Since the window for 5.0.2 is closing very soon, we may also want to merge this and add test coverage later in main.

[ErikEJ]

@roji And there is no way to inspect the perf counters programmatically?

[roji]

It's possible to connect and consume the counters just like the dotnet counters tool does - but see my original comment above with reliability issues etc. It's probably doable, but not sure the effort is worth it (and in any case I wouldn't be comfortable with introducing potentially unreliable tests into a release branch...).

[roji]

In case anyone's interested, here's the incomplete attempt at proper end-to-end coverage: roji@38305bb roji@de7a95b

[ajcvickers]

Punting this out to 5.0.3; we don't need to rush this into 5.0.2.