Migration of Spectator to Micrometer #486

nebhale · 2018-04-06T19:09:00Z

This change migrates the metrics-gathering project from using Spectator to using Micrometer. This change fully moves both the DuplexConnectionInterceptor and RSocketInterceptor to a Micrometer-based implementation, changing some of the metric names. The metric name changes were made at the advice of the project lead of Micrometer in order to get the most useful (and compatible) metrics and dimensions for monitoring an RSocket-based system.

As part of writing the tests, some necessary changes were detected in the framing abstraction. These changes are included in this.

[resolves #434]

yschimke · 2018-04-07T06:09:18Z

rsocket-micrometer/src/main/java/io/rsocket/micrometer/MicrometerDuplexConnection.java

+        case CANCEL:
+          this.cancel.increment();
+          break;
+        case PAYLOAD:


It seems like a mistake to not track payload frames.

The oversight here was actually including the enum and doing nothing with it (ah, IDE's conveniently adding everything). Experimentally I noticed that PAYLOAD frames are never transferred, instead being immediately translated into COMPLETE, NEXT and NEXT_COMPLETE frames. I'll add in a counter for PAYLOAD anyway (since it can't hurt), but I suspect it'll always be 0.

yschimke · 2018-04-07T06:09:33Z

rsocket-micrometer/src/main/java/io/rsocket/micrometer/MicrometerDuplexConnection.java

+      FrameType frameType = frame.getType();
+
+      switch (frameType) {
+        case RESERVED:


Can we let this fall to the default case?

Yep, same oversight as PAYLOAD, letting the IDE get the better of me.

yschimke · 2018-04-07T06:13:00Z

rsocket-micrometer/src/main/java/io/rsocket/micrometer/MicrometerDuplexConnection.java

+          this.extension.increment();
+          break;
+        default:
+          this.logger.warn("Skipping count of unknown frame type: {}", frameType);


I'd suggest this be at debug level and still cause a counter to be incremented. It has always bit me in server development to decide to be verbose about any error that a misconfigured or newer client can externally trigger. Suddenly server performance is effected by excessive logging, while instead tracking a counter and then temporarily increasing logging in response to an alert is much safer IMHO.

Reasonable enough. Going with an UNKNOWN frame type.

yschimke · 2018-04-07T06:18:07Z

rsocket-micrometer/src/main/java/io/rsocket/micrometer/MicrometerRSocket.java

+        MeterRegistry meterRegistry, String interactionModel, SignalType signalType, Tag... tags) {
+
+      return meterRegistry.timer(
+          String.format("rsocket.%s", interactionModel),


nit: String.format here seem like overkill compare to "rsocket." + interactionModel, where interactionModel is just a String.

Stylistically, I prefer to use String.format() consistently for readability, but happy to conform to project conventions.

yschimke · 2018-04-07T06:29:44Z

Looks great to me generally, few nits.

Would you normally split out different clients (either host:port target, or different backend services) by the tags passed to the constructor of MicrometerDuplexConnectionInterceptor?

What is the rollup pattern with Micrometer (e.g. 1 user service backed by 5 different load balanced host:ports)? by ignoring the tag when querying the results from a shared registry?

robertroeser · 2018-04-07T07:44:44Z

rsocket-micrometer/src/main/java/io/rsocket/micrometer/MicrometerRSocket.java

+  }
+
+  private static final class InteractionTimers implements BiConsumer<Sample, SignalType> {
+


One thing I was thinking about adding in something else I was working on was the idea of capturing the time between onNexts. Right now it's really hard to tell how quickly, or slowly a stream is emitting data. Having the time between when the stream starts and cancel/error/complete doesn't really tell that. Throwing this out there as a suggestion as you're refactoring the metrics...

Nice - is that a https://micrometer.io/docs/concepts#_distribution_summaries

Can we iterate on improvements like this? I'd love to get this in sooner and start seeing what results look like from the CLI etc.

Yep - something like that.

Can we iterate on improvements like this? I'd love to get this in sooner and start seeing what results look like from the CLI etc.
Sure

Agreed that's a really useful metric, but also that we should kick the can on it. At some level we're getting rates at the DuplexConnection, but I'll confer with @jkschneider and work on #487 once I have a plan.

@robertroeser I think this onNext timer would be potentially an interesting application of the IntervalTimer we plan to introduce with Micrometer 1.1. It's purpose is to count/time the space between things. Conventionally I've seen it used as something to alert me when say a periodic batch job doesn't kick off in an expected amount of time, but the concept interestingly applies equally well here.

robertroeser · 2018-04-07T07:55:05Z

rsocket-micrometer/src/main/java/io/rsocket/micrometer/MicrometerDuplexConnection.java

+  public Mono<Void> onClose() {
+    return delegate.onClose();
+  }
+


What are people's thoughts on counting the disposes and onCloses? Might help with debugging or alerts

I don't really have enough historical context to know what problem that's trying to solve. I'll defer to the rest of you for a definitive call on collecting them. Just let me know and I'll add it if desired.

I've added onClose print statements to debug issues and I've had similar data about http before so I would like to capture it if no one has an objection.

Will do @robertroeser. My suspicion is that this will only be 0/1. Is that correct?

nebhale · 2018-04-07T14:53:53Z

Would you normally split out different clients (either host:port target, or different backend services) by the tags passed to the constructor of MicrometerDuplexConnectionInterceptor?

Correct. We also see things like what stack (container image, VM id, etc.), region, etc. put in there. I'm toying around with a strategy so that a MicrometerDuplexConnection and MicrometerRSocket knows what it is connected to (other end of the socket) so there's enough tag information to draw the pretty network graphs without requiring another component to collect that information. Not sure it's possible, but we're very keen on getting that kind of graphing up and running for demos.

What is the rollup pattern with Micrometer (e.g. 1 user service backed by 5 different load balanced host:ports)? by ignoring the tag when querying the results from a shared registry?

Exactly. A dimensionally rolled up metric that might filter on some, but specifically not that one host-identifying tag.

jkschneider · 2018-04-07T19:35:17Z

What is the rollup pattern with Micrometer
A dimensionally rolled up metric that might filter on some, but specifically not that one host-identifying tag.

Just to be clear, this kind of behavior is a common feature of dimensional monitoring systems, not Micrometer itself. The primary purpose of Micrometer is to get your metrics to those kind of systems to enable this kind of rollup.

nebhale · 2018-04-08T14:13:09Z

@robertroeser Added metrics for onClose() and dispose() and I believe all other comments have now been addressed.

This change migrates the metrics-gathering project from using Spectator to using Micrometer (https://micrometer.io). This change fully moves both the DuplexConnectionInterceptor and RSocketInterceptor to a Micrometer-based implementation, changing some of the metric names. The metric name changes were made at the advice of the project lead of Micrometer in order to get the most useful (and compatible) metrics and dimensions for monitoring an RSocket- based system. As part of writing the tests, some necessary changes were detected in the framing abstraction. These changes are included in this. [resolves #434]

yschimke reviewed Apr 7, 2018

View reviewed changes

robertroeser reviewed Apr 7, 2018

View reviewed changes

nebhale mentioned this pull request Apr 7, 2018

Collection Micrometer rate metrics at the RSocket stream layer #487

Open

robertroeser merged commit ff4647b into rsocket:1.0.x Apr 9, 2018

nebhale deleted the micrometer branch April 11, 2018 21:46

		}

		private static final class InteractionTimers implements BiConsumer<Sample, SignalType> {

Migration of Spectator to Micrometer #486

Migration of Spectator to Micrometer #486

Uh oh!

Conversation

nebhale commented Apr 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nebhale Apr 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nebhale Apr 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yschimke commented Apr 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertroeser Apr 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nebhale commented Apr 7, 2018

Uh oh!

jkschneider commented Apr 7, 2018

Uh oh!

nebhale commented Apr 8, 2018

Uh oh!

Uh oh!

nebhale Apr 7, 2018 •

edited

Loading

nebhale Apr 7, 2018 •

edited

Loading

yschimke commented Apr 7, 2018 •

edited

Loading

robertroeser Apr 7, 2018 •

edited

Loading