Particular / NServiceBus

Build, version, and monitor better microservices with the most powerful service platform for .NET
https://particular.net/nservicebus/
Other
2.07k stars 650 forks source link

Organize meters by feature #7082

Open mauroservienti opened 3 weeks ago

mauroservienti commented 3 weeks ago

Following one of the NServiceBus design principles, this PR tries to reorganize metrics and meters by feature/concern rather than grouping them all in the same spot as a cross-cutting concern.

The NServiceBus resulting code looks good, and I have no remarks. However, I see a few downsides:

danielmarbach commented 3 weeks ago

I like it. I would even go one step further and put them into the behaviors where they make most sense. Essentially moving them out of the OTEL metrics folder

danielmarbach commented 2 weeks ago

When adding a new metric, no failing tests signal that metrics and meters are part of public API. The problem is that with no failing test, there isn't an easy way also to signal that a new approval test for the new metric must be created

Metrics Factory gives us that API surface

The MetricTags class is scoped for all metrics because it's scoped to the pipeline, which is a mismatch

If that is desirable, partial classes are an easy fix

There's no single class/place to check which metrics we emit

That's a straw man. There is no single class for the pipeline either, and that is by design because Core is componetized.

danielmarbach commented 2 weeks ago

I think there is also assumption in this statement that there needs to be a single test that verifies all meters. That is not necessarily true. In a feature oriented approach, you are validating the meters where they are defined and declared. By following good naming conventions etc. it becomes apparent what is where and you have enough safety in place to not accidentally break things. You could even argue that when you have dedicated tests for meters defined in a feature area you can individually evolve those tests with the changes that are required to those meters. Approval file verifications become then more group and focused around are of change.