[KODO-12017] 添加生产模式输出详细日志功能及配置 #499

yinxulai · 2021-04-16T10:32:23Z

No description provided.

codecov · 2021-04-16T10:34:00Z

Codecov Report

Merging #499 (7187c4b) into master (2d474dc) will increase coverage by 2.21%.
The diff coverage is 42.69%.

@@            Coverage Diff             @@
##           master     #499      +/-   ##
==========================================
+ Coverage   40.00%   42.21%   +2.21%     
==========================================
  Files           7        9       +2     
  Lines         455      514      +59     
  Branches       99      113      +14     
==========================================
+ Hits          182      217      +35     
- Misses        271      285      +14     
- Partials        2       12      +10

Impacted Files	Coverage Δ
src/utils.ts	`17.85% <0.00%> (-0.26%)`	⬇️
src/upload/base.ts	`15.85% <12.12%> (-1.29%)`	⬇️
src/logger/report-v3.ts	`52.38% <52.38%> (ø)`
src/logger/index.ts	`95.83% <95.83%> (ø)`

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 2d474dc...7187c4b. Read the comment docs.

yinxulai · 2021-04-16T10:36:08Z

src/upload/base.ts

        return this.putFile()
      }

      this.onError(err)
-      throw err


MARK：所有的 error 都交给 onError，不应该再 throw 出去

1、是否 throw 到 global 看一下 rxjs 的设定，比如是否跟绑定了 onError 有关
2、如果去掉这个改流程，那就要改 putFile 的定位，
还要检查所有调用 putFile 的地方，以及 putFile 内部所有 return throw 等流程控制点
现在明显有很多问题

要 at 恒哥来瞅一眼么..

RxJS 的意思应该是如果传了 onError，所有的异常都会走 onError
https://github.com/ReactiveX/rxjs/blob/fa0471f96f1b9e8debe59c7eadda63b5b0aef5bd/src/internal/Observable.ts#L99

这里我当时可能是想表达 rejected status，因为 putFile 本身是返回一个 promise, 既然出错了，那这个 promise 的状态如果是 resolved, 感觉就不太合适。
不过这个 promise 其实用户也不会用，只是内部的形式罢了，我看下面 catch 的 error 里也有 onError, 但是没有 throw, 这个应该是漏掉的，所以：

要么补加一下 throw，然后putFile 这里的 error 处理就统一了

把 throw 都去掉，不用关注 putFile 的 promise 状态，毕竟用户也不会用到

putFile 本身是返回一个 promise, 既然出错了，那这个 promise 的状态如果是 resolved, 感觉就不太合适

跟你说的不太一样啊 @yinxulai

lzfee0227

@nighca 太硬核了

lzfee0227 · 2021-04-16T10:36:05Z

src/logger.ts

+  private stack(): string | undefined {
+    const stack = new Error().stack?.split('\n')
+    // 裁掉前三行，显示真正的函数调用点
+    return stack?.slice(3)[0]?.trim()
+  }


logger 自身不会报错吧。。？
@nighca 来看看。。

src/upload/direct.ts

lzfee0227 · 2021-04-16T11:10:21Z

src/upload/base.ts

        return this.putFile()
      }

      this.onError(err)
-      throw err


1、是否 throw 到 global 看一下 rxjs 的设定，比如是否跟绑定了 onError 有关
2、如果去掉这个改流程，那就要改 putFile 的定位，
还要检查所有调用 putFile 的地方，以及 putFile 内部所有 return throw 等流程控制点
现在明显有很多问题

lzfee0227 · 2021-04-16T12:11:42Z

src/upload/base.ts

    }
  }

-  public async putFile(): Promise<utils.ResponseSuccess<UploadCompleteData>> {
+  public async putFile(): Promise<void> {


要不注释一下吧
说现在这个 promise 既不能告知成功与否，也不能说明结束与否
这种东西相对来说不那么常规

src/upload/base.ts

src/logger.ts

lzfee0227 · 2021-04-16T12:16:42Z

src/upload/resume.ts

+      this.logger.error(errorMessage)
+      throw new Error(errorMessage)


有没有觉得有点冗余？
如果是跟 rxjs 的 onError 并列还好
可是这里不是暗示着可以类似

run().catch(err => logger.error(err))

吗 = =？
run 抛出的异常信息，logger 也是可以消费的吧

其他同

日志输出区别于业务代码，我倾向于直接码飞机，可以比较方便定位到。

码飞机是啥？
另外无论从调用栈还是搜代码的角度看，现在都不存在不方便定位的情况了，理由不成立

lzfee0227 · 2021-04-16T12:27:25Z

src/upload/resume.ts

+      try { utils.removeLocalFileInfo(this.getLocalKey()) }
+      catch (error) { this.logger.error(error) }


如果每个 remove 都是异常情况，正常没有主动 remove 的，并且 log 消息都一样，
那是不是只需要封装一个带 log 的 remove 更好？
免得以后再来个 remove 的时候忘记 log 了
反正你又不是写通用 remove 方法，是给 sdk 内部专用的，内部的上下文又都是一致的
（有点像 api 的 toaster 放在哪里的问题 = = 但毕竟这里打 log 跟上报日志是两码事）

其他 utils 类似的，都想想呗

nighca · 2021-04-18T06:19:09Z

src/logger.ts

@@ -0,0 +1,72 @@
+import { createXHR } from './utils'
+
+export type LogLevel = '>=info' | '>=warn' | '>=error' | false


NIP: 在我用过的 logger 的经验中，基本都约定 leve 设置为 INFO 就是表示 >= INFO，不用特别的 >=，参考 log4j、log4js、logrus 等

另外如果是希望关掉，有 LogLevel 'off' / 'OFF' 的做法，我觉得会比 false 稍好点

nighca · 2021-04-18T06:20:52Z

src/logger.ts

+  private stack(): string | undefined {
+    const stack = new Error().stack?.split('\n')
+    // 裁掉前三行，显示真正的函数调用点
+    return stack?.slice(3)[0]?.trim()


如当面提到的，可能需要多验证一些浏览器；或者 stack 干脆不处理

nighca · 2021-04-18T06:27:51Z

src/upload/base.ts

    try {
      this.bucket = utils.getPutPolicy(this.token).bucket
    } catch (e) {
      this.onError(e)
+      logger.error('get bucket from token failed.', e)


NIP: 我看这里需要 logger.error() 的地方跟 this.onError() 的地方基本是重合的，考虑把 logger.error() 的行为做到 this.onError 里？

nighca · 2021-04-18T06:35:12Z

src/upload/index.ts

+    return new Resume(options, handlers, logger)
+  }
+
+  logger.info('file size less than 4M, use Direct.')


NIP: less or equal

nighca · 2021-04-18T06:41:06Z

src/index.ts

@@ -31,12 +28,14 @@ function upload(
    config
  }

+  // 为每个任务创建单独的 Logger
+  const logger = new Logger(token, config?.disableStatisticsReport, config?.debugLogLevel)


Logger constructor 的第二个参数名是 isReport，这边传的是 disable statistics report，是不是反了？

要不要为每个任务的日志带上唯一的标识（类似 tracing 系统的 req id），以便在同时存在多个上传任务时区分不同任务的输出

nighca · 2021-04-18T06:43:47Z

src/logger.ts

+  }
+
+  /**
+   * @param  {any[]} ...args


NIP: TS 不用写 @param？

src/logger.ts

nighca · 2021-04-18T06:50:44Z

src/upload/base.ts

@@ -214,7 +230,7 @@ export default abstract class Base {
  }

  private sendLog(reqId: string, code: number) {
-    this.statisticsLogger.log({


我看 logger 只有 error() 的内容才会发到服务端（report）

所以以前这里的内容会记到服务端，现在不记到服务端了？

nighca · 2021-04-18T06:51:22Z

src/upload/base.ts

        return this.putFile()
      }

      this.onError(err)
-      throw err


要 at 恒哥来瞅一眼么..

yinxulai · 2021-04-19T10:27:06Z

src/logger/index.ts

+   * @param  {unknown[]} ...args
+   * @description 输出 info 级别的调试信息。
+   */
+  info(...args: unknown[]) {


yinxulai · 2021-04-20T03:48:28Z

src/logger/report-v3.test.ts

+  test('test retry', () => {
+    mockXHR.openCount = 0
+    reportV3('token', testData)
+    for (let index = 1; index <= 10; index++) {


10 次是随便定义的，大于传的 retry 次数就行

src/upload/base.ts

lzfee0227 · 2021-04-20T02:53:25Z

src/utils.ts

+  try { localInfo = JSON.parse(localInfoString) }
+  catch {
+    // 本地信息已被破坏，直接删除
+    try { removeLocalFileInfo(localKey) } catch { }


这几个 catch 不 log 一下？
难道不 throw 的东西就不能被 log 了吗…

lzfee0227 · 2021-04-20T03:33:20Z

src/utils.ts

+  try { localInfo = JSON.parse(localInfoString) }
+  catch {
+    // 本地信息已被破坏，直接删除
+    try { removeLocalFileInfo(localKey) } catch { }
    throw new Error(`getLocalFileInfo failed. key: ${localKey}`)


读 localstorage 出错不报错
解析出错报错
这有点不对称，又有点难用啊 = =
还不如都不报错

src/logger/index.ts

lzfee0227 · 2021-04-20T03:38:55Z

src/upload/base.ts

+      this.handleError('file size exceed maximum value 10000G.')
+      return


本来 handleError 里可以直接 throw 这里就不用到处 return
结果你把 putFile 定位改得不能报错、变得更不明确的同时，还让代码变麻烦了…
注释你也没加
我越发觉得你可以把 log 打里面，但是没必要改定位吧

src/logger.ts

src/upload/base.ts

README.md

src/logger/index.ts

lzfee0227

lgtm

nighca · 2021-04-21T03:29:23Z

src/logger/index.ts

+   * @param  {boolean} retry 重试次数，可选，默认为 3。
+   * @description 向服务端上报统计信息。
+   */
+  report(data: V3LogInfo, retry?: number) {


NIP: 这么看的话把 report 逻辑封装到 Logger 里来意义不大了

nighca · 2021-04-21T03:30:03Z

src/upload/base.ts

  /** 分片大小，单位为 MB */
  chunkSize: number
  /** 上传域名协议 */
  upprotocol: 'http:' | 'https:'
  /** 上传区域 */
  region?: typeof region[keyof typeof region]
+  /** 是否禁止静态日志上报 */


这个为啥叫”静态日志“来着..

修正为统计日志上报

src/upload/base.ts

nighca · 2021-04-21T03:32:17Z

src/upload/base.ts

-    this.xhrList.forEach(xhr => xhr.abort())
+    this.logger.info('start cleaning all xhr.')
+    this.xhrList.forEach(xhr => {
+      xhr.onreadystatechange = null


Co-authored-by: Hanxing Yang <nighca@live.cn>

src/upload/base.ts

nighca

🙆‍♂️

nighca · 2021-04-21T03:44:48Z

README.md

@@ -201,6 +201,7 @@ qiniu.compressImage(file, options).then(data => {
    * config.checkByMD5: 是否开启 MD5 校验，为布尔值；在断点续传时，开启 MD5 校验会将已上传的分片与当前分片进行 MD5 值比对，若不一致，则重传该分片，避免使用错误的分片。读取分片内容并计算 MD5 需要花费一定的时间，因此会稍微增加断点续传时的耗时，默认为 false，不开启。
    * config.forceDirect: 是否上传全部采用直传方式，为布尔值；为 `true` 时则上传方式全部为直传 form 方式，禁用断点续传，默认 `false`。
    * config.chunkSize: `number`，分片上传时每片的大小，必须为正整数，单位为 `MB`，且最大不能超过 1024，默认值 4。因为 chunk 数最大 10000，所以如果文件以你所设的 `chunkSize` 进行分片并且 chunk 数超过 10000，我们会把你所设的 `chunkSize` 扩大二倍，如果仍不符合则继续扩大，直到符合条件。
+    * config.debugLogLevel: `INFO` | `WARN` | `ERROR` | `OFF`，允许程序在控制台输出日志，默认为 `OFF`，不输出任何日志，本功能仅仅用于本地调试，不建议在线上环境开启。


本功能仅仅用于本地调试，不建议在线上环境开启

NIP: 也许将来可以考虑结合 process env

我也考虑到了，后面可能会这么做

yinxulai added 2 commits April 16, 2021 18:29

添加生产模式输出详细日志功能及配置

381a904

添加 .vscode 忽略

628894a

qiniu-bot added the size/L label Apr 16, 2021

yinxulai commented Apr 16, 2021

View reviewed changes

lzfee0227 reviewed Apr 16, 2021

View reviewed changes

nighca reviewed Apr 18, 2021

View reviewed changes

优化实现

ee7022b

yinxulai force-pushed the KODO-12017 branch from 9c223c4 to ee7022b Compare April 19, 2021 10:11

补充分片上传的日志信息

39b01b4

yinxulai commented Apr 19, 2021

View reviewed changes

qiniu-bot added size/XL and removed size/L labels Apr 20, 2021

yinxulai commented Apr 20, 2021

View reviewed changes

yinxulai force-pushed the KODO-12017 branch from a11c34a to 5997043 Compare April 20, 2021 03:49

添加单元测试

3d7983d

lzfee0227 reviewed Apr 20, 2021

View reviewed changes

yinxulai force-pushed the KODO-12017 branch from 5997043 to 3d7983d Compare April 20, 2021 03:51

lzfee0227 reviewed Apr 20, 2021

View reviewed changes

src/upload/base.ts Outdated Show resolved Hide resolved

yinxulai added 2 commits April 20, 2021 15:04

添加文档

7adadc7

优化异常输出

84575bc

lzfee0227 reviewed Apr 21, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

src/logger/index.ts Outdated Show resolved Hide resolved

优化文档 & 优化代码

738861c

lzfee0227 previously approved these changes Apr 21, 2021

View reviewed changes

优化 logger test

bf0e92a

yinxulai dismissed lzfee0227’s stale review via bf0e92a April 21, 2021 03:14

nighca reviewed Apr 21, 2021

View reviewed changes

Update src/upload/base.ts

3334fbf

Co-authored-by: Hanxing Yang <nighca@live.cn>

yinxulai commented Apr 21, 2021

View reviewed changes

src/upload/base.ts Outdated Show resolved Hide resolved

静态日志修正为统计日志

7187c4b

nighca approved these changes Apr 21, 2021

View reviewed changes

yinxulai merged commit 850f8bb into qiniu:master Apr 21, 2021

		this.logger.error(errorMessage)
		throw new Error(errorMessage)

		try { utils.removeLocalFileInfo(this.getLocalKey()) }
		catch (error) { this.logger.error(error) }

		@@ -0,0 +1,72 @@
		import { createXHR } from './utils'

		export type LogLevel = '>=info' \| '>=warn' \| '>=error' \| false

		this.handleError('file size exceed maximum value 10000G.')
		return

[KODO-12017] 添加生产模式输出详细日志功能及配置 #499

[KODO-12017] 添加生产模式输出详细日志功能及配置 #499

Conversation

yinxulai commented Apr 16, 2021

codecov bot commented Apr 16, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winddies Apr 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzfee0227 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzfee0227 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nighca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 16, 2021 •

edited

winddies Apr 19, 2021 •

edited