Add support for recording HTTP stats #370
Conversation
Codecov Report
@@ Coverage Diff @@
## master #370 +/- ##
==========================================
- Coverage 95.22% 95.21% -0.02%
==========================================
Files 128 129 +1
Lines 8487 8593 +106
Branches 627 633 +6
==========================================
+ Hits 8082 8182 +100
- Misses 405 411 +6
Continue to review full report at Codecov.
|
|
|
||
| export type HttpGetCallback = (res: httpModule.IncomingMessage) => void; | ||
| export type HttpModule = typeof httpModule; | ||
| export type RequestFunction = typeof httpModule.request; | ||
|
|
||
| // tslint:disable-next-line:no-any | ||
| function isOpenCensusRequest(options: any) { |
There was a problem hiding this comment.
What would you think about making a simple interface for this rather than passing in any here? Seems like it just has an optional headers attribute that is a map of string to string.
There was a problem hiding this comment.
Changed to RequestOptions, instead of any.
| request: httpModule.ClientRequest, options: httpModule.RequestOptions, | ||
| plugin: HttpPlugin): Func<httpModule.ClientRequest> { | ||
| return (span: Span): httpModule.ClientRequest => { | ||
| const startTime = Date.now(); |
There was a problem hiding this comment.
Could this be a high-accuracy timestamp? I believe some Node versions support performance.now(), which would be an easy swap in, or else you could use process.hrtime and do some conversion / use a utility.
There was a problem hiding this comment.
For stats recoding, we want to record ms level granularity. Looks like Date.now() gives us that. WDYT?
There was a problem hiding this comment.
Yes, that's fine. Didn't realize the stats were only at ms granularity.
| @@ -15,3 +15,6 @@ | |||
| */ | |||
|
|
|||
| export * from './http'; | |||
| import {registerAllClientViews, registerAllServerViews, registerAllViews} from './http-stats'; | |||
|
|
|||
| export {registerAllClientViews, registerAllServerViews, registerAllViews}; | |||
There was a problem hiding this comment.
Could this just be export {registerAllClientViews, registerAllServerViews, registerAllViews} from './http-stats';?
mayurkale22
force-pushed
the
http_stats_recording
branch
from
276c15d to
ae0c276
Compare
This PR provides instrumentation for HTTP Client and Server to record HTTP stats. Part of #269
These changes have all been tested locally against prometheus exporter and been confirmed to be working.
I will work on recording stats for send/received bytes in seperate PR.