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
Development

Successfully merging this pull request may close these issues.

3 participants