Added endpoint-level metrics keeping track of the request rate for each path. #104

lducharme · 2018-02-02T23:54:34Z

I added metrics for request rate at the endpoint level. In addition to having this:

com.nordstrom.xrpc.server.Router.requests.Rate
             count = 0
         mean rate = 0.00 events/second
     1-minute rate = 0.00 events/second
     5-minute rate = 0.00 events/second
    15-minute rate = 0.00 events/second

we now also have metrics that keep track of the rate of requests for each endpoint (that is, combination of HTTP method and path):

com.nordstrom.xrpc.server.Router.handler.rate.get.people
             count = 0
         mean rate = 0.00 events/second
     1-minute rate = 0.00 events/second
     5-minute rate = 0.00 events/second
    15-minute rate = 0.00 events/second
com.nordstrom.xrpc.server.Router.handler.rate.post.people.{person}
             count = 0
         mean rate = 0.00 events/second
     1-minute rate = 0.00 events/second
     5-minute rate = 0.00 events/second
    15-minute rate = 0.00 events/second

A few things I'm unsure about and would especially appreciate feedback on:
Is ServiceRateLimiter the right place to configure these metrics? Since the overall server level rate is configured there, that's what I went with, but it seems a little off.

Re: issue 103, these metric names might be contributing to further inconsistency- any thoughts on naming these better?

Note we also may consider adding metrics per client. If we have metrics per client and per endpoint, this could end up being a large/unreadable number of metrics... how might we deal with this?

…ch path.

andyday

let me know if you have questions

andyday · 2018-02-03T00:00:03Z

src/main/java/com/nordstrom/xrpc/server/MetricsUtil.java

+
+  protected static String getMeterNameForRoute(Route route, String httpMethod) {
+    String result = "";
+    if (httpMethod != null) {


we should default this to 'ANY'

andyday · 2018-02-03T00:00:47Z

src/main/java/com/nordstrom/xrpc/server/MetricsUtil.java

+      result += httpMethod.toLowerCase();
+    }
+
+    if (route != null && route.toString() != null) {


no route should cause us to throw a new ArgumentNullException

andyday · 2018-02-03T00:03:12Z

src/main/java/com/nordstrom/xrpc/server/MetricsUtil.java

+    return getMeterNameForRoute(route, httpMethod.name());
+  }
+
+  protected static String getMeterNameForRoute(Route route, String httpMethod) {


since httpMethod can be null we should use Optional<String> httpMethod here

Optional is much better for readability, but it adds extra garbage for each request. I'm planning on running a benchmarking series at some point; it would be good to see how this impacts things.

andyday · 2018-02-03T00:09:16Z

src/main/java/com/nordstrom/xrpc/server/XrpcConnectionContext.java

@@ -46,4 +46,8 @@
      routes = new AtomicReference<>();

  @Getter private final ObjectMapper mapper;
+
+  @Getter
+  private final ConcurrentHashMap<String, Meter> rateMetersByRouteAndMethod =


this should be named something like metersByRoute . rate doesn't apply and Method is unneeded. ideally, method is already part of the route (conceptually)

andyday · 2018-02-03T00:11:03Z

src/main/java/com/nordstrom/xrpc/server/ServiceRateLimiter.java

+    configEndpointLevelRateMeters(metrics, ctx);
+  }
+
+  private void configEndpointLevelRateMeters(


this doesn't belong in this class. it should be done just below the configResponseCodeMeters() in Router

I originally had it where you suggest, in the constructor for Router, but at that point the routes aren't configured yet, so I can't set up meters for each route.

Looking at it again, perhaps a more appropriate place would be to put it would be early in Router.listenAndServe?

andyday · 2018-02-03T00:17:22Z

src/main/java/com/nordstrom/xrpc/server/ServiceRateLimiter.java

+    ImmutableSortedMap<Route, List<ImmutableMap<XHttpMethod, Handler>>> routes =
+        ctx.getRoutes().get();
+
+    final String NAME_PREFIX = "handler.rate.";


this should be changed to something like "routes."; also the name should be changed to namePrefix

andyday · 2018-02-03T00:27:42Z

src/main/java/com/nordstrom/xrpc/server/ServiceRateLimiter.java

+
+    final String NAME_PREFIX = "handler.rate.";
+
+    Set<String> routeAndMethodNames = new HashSet<>();


no need to have this temp holding set. the routes can be added directly to the metric registry

andyday · 2018-02-03T00:29:28Z

src/main/java/com/nordstrom/xrpc/server/ServiceRateLimiter.java

+
+    for (String routeName : routeAndMethodNames) {
+      ctx.getRateMetersByRouteAndMethod()
+          .put(routeName, metricRegistry.meter(name(Router.class, NAME_PREFIX + routeName)));


change to .meter(namePrefix + routeName)... . this includes my other comment about changing NAME_PREFIX to namePrefix

xjdr · 2018-02-05T17:06:29Z

I'd like to see the regressions numbers between this and Master to make sure we aren't adding additional latency with this.

…e names for clarity, used Optional instead of null check.

lducharme · 2018-02-05T18:47:08Z

Thanks for the comments, Andy. I moved the metrics config out of ServiceRateLimiter and into Router.listenAndServe, which is hopefully a better place for it. I also updated the variable names and metric names, and changed MetricsUtil to use Optional to handle possible null parameters.

Let me know if any of this still has issues. I am working on doing benchmarking to check on the latency.

andyday · 2018-02-05T19:16:23Z

src/main/java/com/nordstrom/xrpc/server/MetricsUtil.java

    }

-    return result;
+    return method + routeIdentifier;


this return can happen inside the if, eliminating the need for the routeIdentifier variable. in addition it is a good practice to use inverted if conditions to allow the happy path logic to happen outside of nested conditionals. for instance...

if (route == null || route.toString() == null) { throw new IllegalArgumentException("Route cannot be null."); } return String.format("{}{}", method, route.toString().replace('/', '.');

is better than the alternative you've presented. it allows the happy path to be as unencumbered by levels of nesting while reducing the overall Cyclomatic Complexity of the method.

Makes sense, I updated this.

However, I also added a route identifier map to cache the route identifiers per one of Jesse's comments, so hopefully the way I refactored it still makes sense and reduces complexity.

andyday · 2018-02-05T19:21:30Z

src/main/java/com/nordstrom/xrpc/server/Router.java

@@ -365,6 +369,26 @@ public void listenAndServe(boolean serveAdmin, boolean scheduleHealthChecks) thr
    channel = future.channel();
  }

+  private void configEndpointLevelRateMeters(


I'm not sure that these are accurately Rate meters in the same sense as we use the word Rate in Rate Limiting. Or at minimum they are using different Rates. That being said we should use a different word here to avoid confusion. Some suggestions ThroughPut, RequestCount, Requests....

andyday · 2018-02-05T19:22:55Z

src/main/java/com/nordstrom/xrpc/server/Router.java

+        for (XHttpMethod httpMethod : map.keySet()) {
+          String routeName = MetricsUtil.getMeterNameForRoute(route, httpMethod);
+          ctx.getMetersByRoute()
+              .put(routeName, metricRegistry.meter(name(Router.class, namePrefix + routeName)));


this should use the .meter method that uses only namePrefix + routeName as the name without Router.class

using the class makes the name overly verbose and noisy

Sounds good, I'll remove it.

I originally included the class name because ServiceRateLimiter did it, here:
https://github.com/Nordstrom/xrpc/blob/master/src/main/java/com/nordstrom/xrpc/server/ServiceRateLimiter.java#L56
Does it also make sense to remove the class name there, too?

yes i'd say making the names as short and sweet but unique is the way to go.

great, removed them.

…d with Rate meters. Removed class name from metrics registry.

…o feat-endpoint-level-metrics

lducharme · 2018-02-05T20:27:24Z

I used the run regression script to check out latency in master vs my branch. The max is definitely higher but the average is very close. (Note I had to increase the server and global rate limits for this to work):

master:

  4 threads and 15 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     2.44ms    2.89ms  62.62ms   92.39%
    Req/Sec   312.18    208.79     1.32k    80.91%
  Latency Distribution
     50%    1.70ms
     75%    3.37ms
     90%    4.97ms
     99%    9.21ms
  54451 requests in 1.00m, 3.74MB read
  Socket errors: connect 12, read 54451, write 0, timeout 0
Requests/sec:    905.97
Transfer/sec:     63.70KB

my branch:

  4 threads and 15 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     2.46ms    3.18ms  87.48ms   93.80%
    Req/Sec   312.49    200.63     1.18k    79.12%
  Latency Distribution
     50%    1.68ms
     75%    3.30ms
     90%    4.93ms
     99%    9.37ms
  56996 requests in 1.00m, 3.91MB read
  Socket errors: connect 12, read 56996, write 0, timeout 0
Requests/sec:    948.68
Transfer/sec:     66.70KB

jkinkead · 2018-02-05T22:05:14Z

src/main/java/com/nordstrom/xrpc/server/Http2Handler.java

@@ -88,6 +89,7 @@ private void executeHandler(ChannelHandlerContext ctx, int streamId, Route route
            .findFirst();

    if (handlerMapOptional.isPresent()) {
+      httpRequestMethodName = handlerMapOptional.get().keySet().asList().get(0).name();


On line 81 above, the method name is calculated.

You should be able to speed things up by calculating this once before line 69, and saving the method name in the local.

(you'll speed things up by avoiding the calls here, but also by avoiding re-calculating the method in the inner loop)

jkinkead · 2018-02-05T22:06:46Z

src/main/java/com/nordstrom/xrpc/server/Http2Handler.java

+    // in
+    // Router.serveAdmin()); we do not track metrics for admin endpoints.
+    String meterName = MetricsUtil.getMeterNameForRoute(route, httpRequestMethodName);
+    if (xctx.getMetersByRoute().get(meterName) != null) {


Cache result of this expression:

// too lazy to look up actual type MeterType meter = xctx.getMetersByRoute().get(meterName); if (meter != null) { meter.mark(); }

jkinkead · 2018-02-05T22:07:56Z

src/main/java/com/nordstrom/xrpc/server/MetricsUtil.java

+
+    String routeIdentifier;
+    if (route != null && route.toString() != null) {
+      routeIdentifier = route.toString().replace('/', '.');


This should probably be cached as a property of the route, or in a map somewhere.

jkinkead · 2018-02-05T22:08:28Z

src/main/java/com/nordstrom/xrpc/server/UrlRouter.java

+            // configured in
+            // Router.serveAdmin()); we do not track metrics for admin endpoints.
+            String meterName = MetricsUtil.getMeterNameForRoute(route, request.method().name());
+            if (xctx.getMetersByRoute().get(meterName) != null) {


Cache expression result as above.

…and meters when applicable, reduced redundant calls to calculate method name

…es as succinct and readable as possible.

xjdr · 2018-02-06T17:45:12Z

src/main/java/com/nordstrom/xrpc/server/Http2Handler.java

@@ -108,6 +102,15 @@ private void executeHandler(ChannelHandlerContext ctx, int streamId, Route route
                  .handle(ctx.channel().attr(XrpcConstants.XRPC_REQUEST).get());
    }

+    // Check here for the case of an admin endpoint (eg /metrics, /health, and all others configured
+    // in Router.serveAdmin()); we do not track metrics for admin endpoints.
+    Meter meter =


I'd like to see the functional Optional pattern here like on line 113

Optional<Meter> routeMeter = Optional.ofNullable(xctx.getMetersByRoute() .get(MetricsUtil.getMeterNameForRoute(route, httpRequestMethod.name())); routeMeter.ifPresent(Meter::mark);

Sounds good, changed it in Http2Handler and UrlRouter.

…te meters.

xjdr · 2018-02-07T00:59:53Z

Once you fix the merge conflicts, I am good to approve.

lducharme · 2018-02-07T17:27:32Z

Merge conflicts now fixed (and addressed one more comment that I missed before).

andyday · 2018-02-07T17:47:50Z

👍 All of my remaining comments are unblocking... The one about cyclomatic complexity is worth noting...

andyday · 2018-02-07T17:53:02Z

you still need to fix the build issues. CorsTest is failing...

lducharme · 2018-02-07T19:18:53Z

Build issues fixed! Thanks all for the help

Added endpoint-level metrics keeping track of the request rate for ea…

89e36d9

…ch path.

lducharme requested review from pdex, andyday, jkinkead and xjdr February 2, 2018 23:54

andyday suggested changes Feb 3, 2018

View reviewed changes

Lauren DuCharme and others added 2 commits February 5, 2018 09:54

Moved route metrics config out of ServiceRateLimiter, changed variabl…

b0e5a11

…e names for clarity, used Optional instead of null check.

Merge branch 'master' into feat-endpoint-level-metrics

1e0bacc

andyday suggested changes Feb 5, 2018

View reviewed changes

Lauren DuCharme added 2 commits February 5, 2018 11:44

Changed to 'configEndpointRequestCountMeters' so as not to be confuse…

dd97711

…d with Rate meters. Removed class name from metrics registry.

Merge remote-tracking branch 'origin/feat-endpoint-level-metrics' int…

36b41e0

…o feat-endpoint-level-metrics

jkinkead reviewed Feb 5, 2018

View reviewed changes

Lauren DuCharme added 2 commits February 5, 2018 16:15

Made some performance fixes per Jesse's comments: cached route names …

cac24ed

…and meters when applicable, reduced redundant calls to calculate method name

Removed class name from metrics counter so as to make the metrics nam…

2d73eb3

…es as succinct and readable as possible.

xjdr reviewed Feb 6, 2018

View reviewed changes

Instead of null check, changed to functional optional pattern for rou…

e699dbc

…te meters.

Lauren DuCharme added 3 commits February 7, 2018 09:06

Merge branch 'master' into feat-endpoint-level-metrics

f9c010e

Moved MetricsUtil into correct package after folder restructure

93ccbbf

Inverted if conditions to be clearer and less complex.

004743d

andyday approved these changes Feb 7, 2018

View reviewed changes

Lauren DuCharme added 2 commits February 7, 2018 09:53

Removed nested conditional

741906d

added null check to fix NPE during CorsTests

37b3bd3

lducharme merged commit e4fca46 into master Feb 7, 2018

lducharme deleted the feat-endpoint-level-metrics branch February 7, 2018 19:19

This was referenced Feb 9, 2018

Add per-handler timer metrics #121

Closed

New routes implementation #120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added endpoint-level metrics keeping track of the request rate for each path. #104

Added endpoint-level metrics keeping track of the request rate for each path. #104

lducharme commented Feb 2, 2018

andyday left a comment

andyday Feb 3, 2018

andyday Feb 3, 2018

andyday Feb 3, 2018

jkinkead Feb 6, 2018

andyday Feb 3, 2018

andyday Feb 3, 2018

lducharme Feb 3, 2018

andyday Feb 3, 2018

andyday Feb 3, 2018

andyday Feb 3, 2018

xjdr commented Feb 5, 2018

lducharme commented Feb 5, 2018

andyday Feb 5, 2018

lducharme Feb 7, 2018

andyday Feb 5, 2018

andyday Feb 5, 2018

andyday Feb 5, 2018

lducharme Feb 5, 2018

andyday Feb 6, 2018

lducharme Feb 6, 2018

lducharme commented Feb 5, 2018

jkinkead Feb 5, 2018

jkinkead Feb 5, 2018

jkinkead Feb 5, 2018

jkinkead Feb 5, 2018

jkinkead Feb 5, 2018

xjdr Feb 6, 2018

lducharme Feb 6, 2018

xjdr commented Feb 7, 2018

lducharme commented Feb 7, 2018

andyday commented Feb 7, 2018 •

edited

Loading

andyday commented Feb 7, 2018

lducharme commented Feb 7, 2018


		final String NAME_PREFIX = "handler.rate.";

		Set<String> routeAndMethodNames = new HashSet<>();

Added endpoint-level metrics keeping track of the request rate for each path. #104

Added endpoint-level metrics keeping track of the request rate for each path. #104

Conversation

lducharme commented Feb 2, 2018

andyday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xjdr commented Feb 5, 2018

lducharme commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lducharme commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xjdr commented Feb 7, 2018

lducharme commented Feb 7, 2018

andyday commented Feb 7, 2018 • edited Loading

andyday commented Feb 7, 2018

lducharme commented Feb 7, 2018

andyday commented Feb 7, 2018 •

edited

Loading