Parity with Java for Metrics #54
Conversation
Signed-off-by: Christian Weiss <[email protected]>
| public class MetricsImpl : IMetrics | ||
| { | ||
| /// <inheritdoc/> | ||
| [Metric(name: "traces", tag1Key: "state", tag1Value: "started", tag2Key: "sampled", tag2Value: "y")] |
There was a problem hiding this comment.
Is this a limitation of the annotation syntax that you can't do a dictionary?
If so I would recommend just having a string like tags="state=started, sampled=y" and parse it during construction. The objective is to make the annotation readable & easy, not to help the parsing engine (which isn't that complicated anyway).
There was a problem hiding this comment.
Yes, this is a limitation in C#! I'll change it to your recommendation.
| @@ -28,7 +28,11 @@ private RemoteReporter(ITransport transport, ILoggerFactory loggerFactory, IMetr | |||
| try | |||
| { | |||
| int n = await _transport.CloseAsync(CancellationToken.None).ConfigureAwait(false); | |||
| _metrics.ReporterSuccess.Inc(n); | |||
|
|
|||
| if (n > 0) | |||
There was a problem hiding this comment.
does it matter if it's 0? seems like unnecessary complication of the code
| { | ||
| Assert.IsType<InMemoryMetricsFactory.InMemoryElement>(reporter._metrics.ReporterSuccess); | ||
| } | ||
| metrics.ReporterSuccess.Received(1).Inc(2); |
There was a problem hiding this comment.
why not use in-memory implementation and just assert the final counter value? With a mock you are testing not only external side effect, i.e. the resulting value of the counter which is what we're interested in, but also how it gets there, which is mostly an irrelevant implementation detail that makes the test itself more brittle.
|
Superseded by #55. |
I'm comparing the C# code with the Java code and I'm trying to get it as close as possible to make future changes easier. (#29)
This PR brings the "Metrics" feature closer to Java. However there still need to be a few differences:
Metricswhich is used everywhere. Having a class with this name is not nice in C# because the namespace has the same name and people would have to useMetrics.Metrics. As I couldn't come up with a different name, I just kept the already existingIMetricsinterface and moved Java's logic to a newMetricsImplclass.I also replaced the metrics unit tests with the current version from Java.