-
Notifications
You must be signed in to change notification settings - Fork 57
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.
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
feat: monitor http server request by diagnostics_channel #248
Conversation
Usage: ```js XPROFILER_PATCH_HTTP_WITH_DIAGNOSTICS_CHANNEL=YES npm start ``` closes X-Profiler/egg-xtransit#10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR adds support for monitoring HTTP server requests via diagnostics_channel. Key changes include:
- Adding new subscription functions (subscribeHttpServerRequestStart and unsubscribeHttpServerRequestStart) that leverage diagnostics_channel.
- Updating configuration, tests, and documentation to support the new patch_http_with_diagnostics_channel flag.
- Adjusting patch logic in patch/index.js to conditionally use diagnostics_channel based on configuration.
Reviewed Changes
File | Description |
---|---|
test/patch/http_with_diagnostics_channel.test.js | New tests to validate the diagnostics_channel integration. |
patch/http.js | Added subscription/unsubscription functions and refactored HTTP patch. |
configuration.js | Extended configuration to include the new patch_http_with_diagnostics_channel flag. |
patch/index.js | Updated patch logic to toggle between diagnostics_channel and default patch. |
test/fixtures/cases/config.js, logbypass.js, command.js | Updated test fixtures to cover the new configuration flag. |
README.md | Documentation updated with details about diagnostics_channel support. |
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
test/fixtures/cases/logbypass.js:138
- The configuration value for patch_http_with_diagnostics_channel is set to 0, which may cause type coercion issues. It is recommended to use a proper boolean value (false) to maintain consistency with the configuration schema.
XPROFILER_PATCH_HTTP_WITH_DIAGNOSTICS_CHANNEL: 0,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR adds support for monitoring HTTP server requests using the diagnostics_channel module when enabled via configuration. Key changes include:
- Adding new functions subscribeHttpServerRequestStart and unsubscribeHttpServerRequestStart in patch/http.js.
- Updating configuration and tests with the new patch_http_with_diagnostics_channel option.
- Adjusting the patch logic in patch/index.js to conditionally apply diagnostics_channel-based patching.
Reviewed Changes
File | Description |
---|---|
test/patch/http_with_diagnostics_channel.test.js | Adds tests for monitoring HTTP requests using diagnostics_channel |
patch/http.js | Introduces subscribeHttpServerRequestStart/unsubscribeHttpServerRequestStart functions and exports them |
configuration.js | Adds new configuration option patch_http_with_diagnostics_channel |
test/fixtures/cases/config.js | Adds test case for the new configuration option |
patch/index.js | Adjusts patch logic to switch between diagnostics_channel and standard patching |
.github/workflows/nodejs.yml | Removes scheduled trigger for CI jobs |
test/fixtures/cases/logbypass.js | Includes the new environment variable for diagnostics channel patching |
test/fixtures/cases/command.js | Updates command configuration expectations with the new option |
README.md | Documents the new configuration option |
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This pull request introduces an optional HTTP monitoring feature that leverages the diagnostics_channel module to capture HTTP request metrics. It updates the patching logic, configuration options, tests, and documentation to support the new patch_http_with_diagnostics_channel flag.
- Updated patch/http.js to export and use subscribe/unsubscribe functions for diagnostics_channel.
- Revised configuration and test fixtures to include the new diagnostics channel-based patch.
- Updated CI workflows and documentation to reflect the change.
Reviewed Changes
File | Description |
---|---|
test/patch/http_with_diagnostics_channel.test.js | New tests for verifying diagnostics_channel-based HTTP request monitoring. |
.github/workflows/release.yml | Added release workflow configuration. |
patch/http.js | Added subscribeHttpServerRequestStart and unsubscribeHttpServerRequestStart functions, and updated exports. |
configuration.js | Introduced new configuration for patch_http_with_diagnostics_channel. |
patch/index.js | Updated patch logic to conditionally use diagnostics_channel subscription. |
test/fixtures/cases/config.js | Added diagnostics_channel configuration in test cases. |
.github/workflows/nodejs.yml | Updated Node.js setup to a newer version. |
test/fixtures/cases/logbypass.js | Adjusted environment variable value for diagnostics_channel patch in tests. |
test/fixtures/cases/command.js | Updated test commands to check diagnostics_channel-related configuration. |
README.md | Documented the new diagnostics_channel patch option. |
lib/utils.js | Minor formatting change. |
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
@hyj1991 node 12 和 14 没法坚持了,github action 都安装不了了。 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #248 +/- ##
============================================
- Coverage 100.00% 29.12% -70.88%
============================================
Files 10 10
Lines 321 309 -12
============================================
- Hits 321 90 -231
- Misses 0 219 +219 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
我再重构一下,改成只用 diagnostics_channel 监控 http server request 统计,并且不再支持 Node.js < 18,发一个 major 版本 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR introduces a new monitoring feature that leverages diagnostics_channel for HTTP server request tracking and drops support for Node.js versions earlier than 18.19.0. Key changes include updates to GitHub workflow configurations (e.g. supported Node.js versions and action versions), modifications to the HTTP instrumentation code in the patch module to use diagnostics_channel subscriptions, and corresponding test updates and documentation changes.
Reviewed Changes
File | Description |
---|---|
.github/workflows/release.yml | Added release workflow configuration |
README.md | Updated documentation and supported Node.js runtime version info |
.github/workflows/nodejs.yml | Updated GitHub Action configuration and supported node versions |
patch/index.js | Switched from patchHttp to subscribeHttpServerRequestStart |
test/patch/http.test.js | Adjusted tests to reflect new diagnostic_channel behavior |
test/utils.test.js | Minor formatting fix with semicolon |
patch/http.js | Refactored HTTP instrumentation to use diagnostics_channel |
lib/utils.js | Added missing semicolon for consistency in code formatting |
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
patch/http.js:29
- [nitpick] Consider renaming 'patch_http_timeout' to a more descriptive name like 'httpPatchTimeout' to clearly indicate its purpose as a timeout value used in HTTP instrumentation.
}, patch_http_timeout * 1000);
@@ -125,7 +125,7 @@ string GetOsVersion() { | |||
LPSERVER_INFO_101 os_info = NULL; | |||
NET_API_STATUS nStatus = NetServerGetInfo(NULL, level, (LPBYTE*)&os_info); | |||
if (nStatus == NERR_Success) { | |||
LPSTR os_name = "Windows"; | |||
std::string os_name = "Windows"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This pull request implements monitoring of HTTP server requests by leveraging the diagnostics_channel API and drops support for Node.js versions below 18.19.0. Key changes include:
- Replacing the custom HTTP patching mechanism with diagnostics_channel subscriptions.
- Updating GitHub Action workflows to reflect new Node.js version requirements and upgrade action versions.
- Adjusting documentation and tests to align with the new behavior.
Reviewed Changes
File | Description |
---|---|
.github/workflows/release.yml | Added release workflow configuration. |
README.md | Updated documentation to reflect new monitoring and version support. |
.github/workflows/nodejs.yml | Removed outdated schedule and reduced supported node versions. |
patch/index.js | Updated patching logic to use diagnostics_channel subscribe API. |
test/patch/http.test.js | Adjusted tests to match the new HTTP request monitoring behavior. |
test/utils.test.js | Minor syntax update for consistency. |
patch/http.js | Refactored HTTP patching to diagnostics_channel with subscribe/unsubscribe functions. |
lib/utils.js | Minor code style correction. |
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
README.md:80
- The term 'FataLError' appears to be a typo; consider using 'FatalError' for clarity.
**enable_fatal_error_report**: 是否需要在 V8 出现 FataLError 时导出 Report 文件, 默认 `true`
patch/http.js:15
- [nitpick] Consider using a more descriptive name for '_onHttpServerRequestStart' to clearly indicate its role as the diagnostics_channel subscription handler.
let _onHttpServerRequestStart;
[skip ci] ## [3.0.0](v2.6.1...v3.0.0) (2025-03-10) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support closes X-Profiler/egg-xtransit#10 ### Features * monitor http server request by diagnostics_channel ([#248](#248)) ([293abe8](293abe8))
BREAKING CHANGE: drop Node.js < 18.19.0 support
closes X-Profiler/egg-xtransit#10