Collect http_response_code for successfull and failed requests

[velo]

On previous implementation response code would only be collected for errors.

To avoid mixing new and old metrics, gave the one that includes successfull codes a different name

[kdavisk6]

@velo

Should we consider using Timers instead of Counters? Timer implicitly count and can provide more information. Thoughts?

[velo]

We already have timers for method handler, client, encoder and decoder. At the time we start the timers we don't know the response code, so can't include that piece of information on timers

…

On Sat, 13 Mar 2021, 03:38 Kevin Davis, ***@***.***> wrote: ***@***.**** commented on this pull request. @velo <https://github.com/velo> Should we consider using Timers instead of Counters? Timer implicitly count and can provide more information. Thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1375 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABBLDS34MGANR4OCI47ZWDTDIKNBANCNFSM4ZBRHF5A> .

[kdavisk6]

Ahh, but with Micrometer you can.

long start = System.nanoTime();  // record the start time.
Response response = client.execute(Request); // execute the request
registry.timer(metric.name).tag(getTags(response)).record(System.nanoTime() - start, TimeUnit.NANOSECONDS) // create a new timer with the time calculated and use getTags() to read the response and set the Success or Failure

This way, you can include a tag that indicates if the request was successful or not. You can use this tag in your metrics system to calculate success and failure rates, without needed a separate counter. Not sure how to do it with Dropwizard.

[velo]

Ahh, but with Micrometer you can.

long start = System.nanoTime();  // record the start time.
Response response = client.execute(Request); // execute the request
registry.timer(metric.name).tag(getTags(response)).record(System.nanoTime() - start, TimeUnit.NANOSECONDS) // create a new timer with the time calculated and use getTags() to read the response and set the Success or Failure

This way, you can include a tag that indicates if the request was successful or not. You can use this tag in your metrics system to calculate success and failure rates, without needed a separate counter. Not sure how to do it with Dropwizard.

Yeah, you can do "exactly" the same on dropwizard metrics.... but ...

      return meterRegistry.timer().recordCallable(() -> {...});

Looks much better than

long start = System.nanoTime();
try {
   Response response = delegate.execute();
   meterRegistry.timer( tag(response.getStatus()) ).record(System.nanoTime() - start, TimeUnit.NANOSECONDS);
   return response;
} catch (FeignException e) {
   meterRegistry.timer( tag(e.getStatus()) ).record(System.nanoTime() - start, TimeUnit.NANOSECONDS);
   throw e;
}

Also, if something outside FeignException happens, you won't get neither the timer nor the status code.

Do you have any specific use case for this or just an improvement?

[velo]

@kdavisk6 also, do you mind if I merge as is, kick an release and if need be we rework this later?!

Cheers,

[kdavisk6]

Could refactor with a try/finally instead. Sounds like you need this for something you are working on. I’ll approve and think on it some more.

[velo]

Awesome, thanks @kdavisk6

I will make a new release for this...

shall we go with 11.1 (as I see people using 11.0 out there, or 10.x?

[kdavisk6]

Yeah. Might as well fix the 11.x problem.