Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: logbypass on worker threads #154

Merged
merged 6 commits into from
Mar 29, 2022
Merged

feat: logbypass on worker threads #154

merged 6 commits into from
Mar 29, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Mar 22, 2022 •
edited
Loading

  • define the log format

@codecov
Copy link

codecov bot commented Mar 22, 2022 •
edited
Loading

Codecov Report

Merging #154 (5c388b2) into master (9a59ce8) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 5c388b2 differs from pull request most recent head d1b9dae. Consider uploading reports for the commit d1b9dae to get more accurate results

@@            Coverage Diff            @@
##            master      #154   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          296       297    +1     
=========================================
+ Hits           296       297    +1
Impacted Files Coverage Δ
xprofiler.js 100.00% <100.00%> (ø)

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 9a59ce8...d1b9dae. Read the comment docs.

hyj1991
hyj1991 reviewed Mar 22, 2022
View reviewed changes
test/log.test.js Outdated
@@ -42,7 +42,8 @@ function parseXprofilerLog(type, content) {
obj.detail = matched[4];
} else {
obj.version = matched[4];
obj.detail = matched[5];
obj.tid = matched[5];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tid 直接放 pid 后面吧，这个本来也准备 break 发 2.0 了

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tid 不是所有日志都会打印的，这样可选的项目放在中间的位置会比较难 parse。或者把 version 提前？

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果是这样的话，那先放最后好了

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

现在这个布局的话，已经产生 break 了吧？
也就是还是需要发 2.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tid 在什么情况下可能没有呢？

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

更新了，现在所有的日志都会有 tid，默认为 0

@legendecas legendecas marked this pull request as ready for review March 25, 2022 03:28
@hyj1991 hyj1991 merged commit 6242c54 into X-Profiler:master Mar 29, 2022
@legendecas legendecas deleted the main-thread branch March 29, 2022 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hyj1991 hyj1991 hyj1991 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@legendecas @hyj1991