Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Feat/refactor http hook #244

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

myfjdthink
Copy link

我拓展了 HttpServerPatcher 和 HttpClientPatcher,让这两个 Hook 都能记录 query、data 和 response。
具体实现可见:
https://github.com/kaolalicai/klg-tracer/blob/master/src/patch/HttpServer.ts
https://github.com/kaolalicai/klg-tracer/blob/master/src/patch/shimmers/http-client/Shimmer.ts

拓展的过程有些小麻烦,主要是上述两个组件的代码结构会有不方便拓展的地方,导致我不能简单复写某些方法,表现有:
1 HttpServerPatcher.shimmer 函数过长,如果要替换其中某段逻辑不方便
2 KlgHttpClientShimmer.wrapRequest 是以成员属性的方式定义的,overwrite 后 super.wrapRequest 会有问题。

除了拓展的问题,我在整理代码的时候发现像

shimmer.wrap(req, 'emit', function wrapRequestEmit (emit) {
        const bindRequestEmit = traceManager.bind(emit)
        return function wrappedRequestEmit (this: IncomingMessage, event) {
          if (event === 'data') {
            // do some thing
          }
          return bindRequestEmit.apply(this, arguments)
        }
      })

这类的钩子其实可以直接用事件监听来实现, 如下,而且没问题,测试通过

req.once('data', (data) => {
       // do some thing
})

针对上述问题,我按照我的想法把 HttpServer 和 KlgHttpClientShimmer 整理了一遍,勉强做个抽象,发了个 PR,供参考。

HttpServer 大致代码结构:

shimmer
    const {tracer, span} = self.initTracerAndSpan(req, res);
    self.wrapRequest(req, res, tracer, span){
      self.recordQuery(req, res, tracer, span);
      self.recordBodyData(req, res, tracer, span);
      // TODO recordResponse
    }
    self.handleResponse(req, res, tracer, span);   // finish tracer and span

KlgHttpClientShimmer 大致代码结构:

httpRequestWrapper
      const {tracer, span} = self.initTracerAndSpan(req, res);
      _request.once('error', (res) => {
        self.handleError(span, res);  // set span error
      });
      
      _request.once('response', (res) => {
        self.wrapRequest(_request, res, tracer, span){
          // TODO this.recordQuery(req, tracer, span);
          // TODO this.recordData(req, tracer, span);
          this.recordResponseWrap(req, res, tracer, span);
        }
        self.handleResponse(_request, res, tracer, span); // finish span
      });

@codecov-io
Copy link

Codecov Report

Merging #244 into master will decrease coverage by 0.06%.
The diff coverage is 89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
- Coverage    80.7%   80.63%   -0.07%     
==========================================
  Files         190      190              
  Lines        6415     6415              
  Branches      870      866       -4     
==========================================
- Hits         5177     5173       -4     
- Misses        853      856       +3     
- Partials      385      386       +1
Impacted Files Coverage Δ
packages/hook/src/patch/HttpClient.ts 95% <ø> (ø) ⬆️
packages/metrics/src/domain.ts 100% <ø> (ø) ⬆️
packages/hook/src/patch/HttpServer.ts 86.81% <86.44%> (+0.06%) ⬆️
...ges/hook/src/patch/shimmers/http-client/Shimmer.ts 81.57% <92.68%> (-0.39%) ⬇️
packages/pandora/src/application/ScalableMaster.ts 79.68% <0%> (-3.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bd1372...5834fbc. Read the comment docs.

@mariodu
Copy link
Member

mariodu commented May 10, 2018

逻辑调整我看下,这块以后会通过规范调整

@mariodu
Copy link
Member

mariodu commented May 10, 2018

下周估计规范接口会集成进来,到时候一起改掉吧

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
3 participants