Adding custom statter option for clients to optionally use #36
Conversation
The custom statter will allow the client to receive all the metrics that Rye calculates around a request. If a custom statter is supplied, it must contain a ReportStats method which will then recieve information around the request, response, as well as the duration of the request handler.
There was a problem hiding this comment.
I was mentioning to @caledhwa that I really dig this approach. Abstracting any potential interfaces for metrics reporters behind our own extra general interface is probably how it should have been done originally. As is, we married Rye a little too closely to the original StatsD lib and made it less extensible as a result.
I would like to hear what my teammates have to say, but this has a soft approval from me. I think there is a conversation that could be had here to remove the original Statter from Rye entirely and maybe just have this new CustomStatter iface be the main one, with possibly a pre-loaded StatsD-ready implementation of this iface that a user could opt to use without creating their own struct to satisfy it. On the other hand if we keep the original Statter in addition to this new one, we take nothing away from Rye's current functionality and add the ability to tee out metrics to StatsD AND/OR another sink.
Just my thoughts.
There was a problem hiding this comment.
+1 Jeff makes a great case for your PR. I fully agree. I also don't want to introduce breaking change so I'm all for leaving the existing Config with the existing statsd Statter. That being said, I assume that your implementation of the custom reporter is to both do timing and counting? You have a single func you're representing.
|
The single func passes down data to the client (if it's set) and they can implement the FWIW, I was planning to parse the URL out from the request and not use the handler name, but after looking at this again, it would probably be nice to pass the handler name down and make this a little bit friendler so that the client won't have to derive that. Will update the PR to pass that along as well. |
Codecov Report
@@ Coverage Diff @@
## master #36 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 10 10
Lines 259 261 +2
=====================================
+ Hits 259 261 +2
|
|
@caledhwa @jescochu - Added the handlerName to the ReportStats method to make it easier for the client to build metrics around it. Can I get another CR on this please? |
rye.go
Outdated
| // CustomStatter allows the client to log any additional statsD metrics Rye | ||
| // computes around the request handler. | ||
| type CustomStatter interface { | ||
| ReportStats(string, time.Duration, *http.Request, *Response) error |
There was a problem hiding this comment.
Nit: I would name each arg in the interface - it makes the purpose more obvious.
As in:
ReportStats(name string, elapsed time.Duration, req *http.Request, resp *Response) error
The custom statter option will allow the client to receive all the metrics that Rye calculates around a request. The custom statter must contain a ReportStats method which will then receive information around the request, response, as well as the duration of the request handler. This is ideal for clients that would prefer to use the
datadog-golibrary or any other library. Additionally, this is a potential option to move the Rye middleware away from any kind of statsD reporting.