feat: report connector version information#102
Conversation
Summary of ChangesHello @qevolg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving the security and observability of the Node.js client. It introduces a robust mechanism to redact sensitive data from logs, ensuring that credentials are not inadvertently recorded. Additionally, it enhances connection transparency by reporting the client's version, which will be valuable for diagnostics. These changes are supported by new unit tests and general code maintenance. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Node.js WebSocket connector to (1) attach a connector identification string (including package version) on connect, and (2) redact sensitive fields in log output, with accompanying tests and minor cleanup.
Changes:
- Enable
resolveJsonModuleto allow importingpackage.jsonin TypeScript. - Add
connectormetadata to the WebSocket connect payload and add an integration test validating it appears inshow connections. - Add
redactMessage()to sanitize logs (password/token/URL credentials) and add unit tests for redaction.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| nodejs/tsconfig.json | Enables JSON imports to support reading connector version metadata. |
| nodejs/src/client/wsClient.ts | Imports package version and includes connector info in connect args. |
| nodejs/src/common/log.ts | Adds redactMessage() and applies it to winston output formatting. |
| nodejs/test/bulkPulling/log.test.ts | Adds unit tests for redactMessage() behavior. |
| nodejs/test/bulkPulling/sql.test.ts | Adds integration test verifying connector info appears in server connections. |
| nodejs/src/client/wsConnectorPool.ts | Minor log formatting; SIGINT/SIGTERM handlers updated. |
| nodejs/src/tmq/wsTmq.ts | Removes unused import / formatting cleanup. |
| nodejs/src/tmq/tmqResponse.ts | Removes commented-out code. |
| nodejs/src/tmq/config.ts | Removes commented-out field. |
| nodejs/src/common/taosResult.ts | Removes unused import. |
| nodejs/src/client/wsResponse.ts | Removes outdated file header comment / formatting cleanup. |
| nodejs/src/client/wsEventCallback.ts | Formatting cleanup for static map initialization. |
| nodejs/prepare.js | Minor formatting (blank line). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two main features: sending connector version information during connection and redacting sensitive data (passwords, tokens) from logs for improved security. While the intention to redact sensitive data is positive, the implementation of the redactMessage function contains several flaws in its regular expressions, leading to incomplete redaction. Specifically, passwords in URLs without usernames are not redacted, passwords containing escaped quotes or '@' characters are only partially redacted, and bearer tokens in string logs are not redacted. Additionally, there is a minor copy-paste error in a log message and a potentially flaky test that uses a fixed sleep. Addressing these points will significantly improve the robustness and security of the changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
==========================================
- Coverage 80.21% 80.21% -0.01%
==========================================
Files 30 30
Lines 2482 2487 +5
Branches 436 437 +1
==========================================
+ Hits 1991 1995 +4
Misses 378 378
- Partials 113 114 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
qevolg
changed the title
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to send connector information, including the version from package.json, when establishing a WebSocket connection. This is implemented for both regular SQL connections and TMQ consumers. The changes are well-tested with new test cases that verify the connector information is correctly reported in show connections.
The PR also includes several valuable code cleanups, such as removing unused imports, commented-out code, and fixing a bug in a log message within a SIGTERM handler.
I have a couple of suggestions to improve the reliability of the new tests by replacing fixed-time waits with a polling mechanism to avoid potential flakiness. Overall, this is a solid contribution.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- nodejs/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…0 environment variable
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to report the connector version to the TDengine server. The implementation correctly adds the connector information, including the version from package.json, to the connection arguments for both SQL and TMQ clients. The accompanying tests verify this new behavior. My feedback focuses on improving the reliability of the new tests by replacing fixed delays with a more robust polling mechanism to avoid potential flakiness.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- nodejs/package-lock.json: Language not supported
Comments suppressed due to low confidence (2)
nodejs/test/bulkPulling/sql.test.ts:336
- This test uses a fixed
Sleep(2000)before checkingshow connections, which makes the suite slower and can still be flaky on slower/faster environments. Prefer pollingshow connectionsuntil the expected row appears (with an overall timeout) instead of an unconditional sleep.
const wsSql = await WsSql.open(conf);
await Sleep(2000);
const wsRows = await wsSql.query("show connections");
nodejs/test/bulkPulling/tmq.test.ts:524
- This test uses a fixed
Sleep(2000)before queryingshow connections, which adds latency and can still be timing-flaky. Prefer polling until the expected connector string appears (with an overall timeout) rather than sleeping unconditionally.
conf.setPwd(testPassword());
const wsSql = await WsSql.open(conf);
await Sleep(2000);
const wsRows = await wsSql.query("show connections");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
qevolg
force-pushed
the
feat/main/6666017184
branch
from
d59c2d7 to
38b736d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to report connector version information to the server by adding a connector field in the connection message for both SQL and TMQ connections. The connector information string includes the version from package.json. The changes are well-implemented and include tests. My feedback focuses on improving the test suite's reliability by replacing fixed-time sleeps with a polling mechanism, and on using modern TypeScript syntax for JSON imports.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- nodejs/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zitsen
left a comment
zitsen
left a comment
There was a problem hiding this comment.
Code Review Summary
Approval: ✅ APPROVED
This PR successfully adds connector version reporting functionality to the TDengine Node.js WebSocket connector. The implementation is clean, well-tested, and follows good practices.
Key Changes
- Added
ConnectorInfoconstant with format:nodejs-ws-v{version}-ncid000 - Integrated version reporting in both SQL and TMQ connection paths
- Added comprehensive tests verifying connector info appears in
show connections - Version bump to 3.2.3
- Enabled
resolveJsonModulein TypeScript config
Strengths
- Clean, focused implementation with minimal changes
- Good test coverage for both connection types
- Proper constant definition and reuse
- Well-integrated into existing connection flow
Tests Verified
- SQL connection shows connector version info
- TMQ connection shows connector version info
- Conditional test execution with TEST_3360 environment variable
The code quality is solid and the feature is production-ready. LGTM! 🚀
Description
feat: report connector version information
Issue(s)
Checklist
Please check the items in the checklist if applicable.