Skip to content

Conversation

@durran
Copy link

@durran durran commented Nov 12, 2025

Pull Request

The PR incorporates MongoDB's wrapping client library specification for the connection handshake to allow library details to be included in the metadata written to mongos or mongod logs.

Approach

Appends client metadata when mongo clients are created.

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK

Summary by CodeRabbit

  • Chores
    • Version metadata is now attached to MongoDB client connections in database adapters.

@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title feat(mongodb): add mongodb client metadata feat(mongodb): Add mongodb client metadata Nov 12, 2025
@parse-github-assistant
Copy link

parse-github-assistant bot commented Nov 12, 2025

🚀 Thanks for opening this pull request!

@coderabbitai
Copy link

coderabbitai bot commented Nov 12, 2025

📝 Walkthrough

Walkthrough

Version metadata is appended to MongoDB clients during connection initialization in two adapter modules. Each adapter adds version information from package.json with a distinct metadata identifier during client setup, without modifying core connection or error-handling logic.

Changes

Cohort / File(s) Summary
MongoDB Client Metadata
src/Adapters/Files/GridFSBucketAdapter.js, src/Adapters/Storage/Mongo/MongoStorageAdapter.js
Added import of version from package.json and appended metadata to MongoDB client during connection setup. Each adapter assigns a unique metadata name ('parse_server_gridfs' and 'parse_server_storage' respectively) paired with the imported version.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Both changes follow an identical pattern (import version, attach metadata during client initialization)
  • Minimal, non-invasive edits with consistent structure across files
  • No modifications to public function signatures or control flow
  • Consider verifying that the metadata attachment does not impact MongoDB driver behavior or client operations

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is mostly complete with a clear approach and relevant context, but the critical 'Issue' section is missing the required issue link. Add the required 'Closes: [ISSUE_LINK]' section to link this PR to its corresponding GitHub issue, as specified in the template.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(mongodb): Add mongodb client metadata' clearly and concisely summarizes the main change - adding metadata to MongoDB clients for telemetry identification.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@parseplatformorg
Copy link
Contributor

parseplatformorg commented Nov 12, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@durran durran marked this pull request as ready for review November 12, 2025 17:02
@mtrezza
Copy link
Member

mtrezza commented Nov 13, 2025

Could you briefly describe the purpose?

@codecov
Copy link

codecov bot commented Nov 13, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.51%. Comparing base (94cee5b) to head (de77d6b).
⚠️ Report is 1 commits behind head on alpha.

Files with missing lines Patch % Lines
src/Adapters/Files/GridFSBucketAdapter.js 50.00% 1 Missing ⚠️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 50.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (94cee5b) and HEAD (de77d6b). Click for more details.

HEAD has 8 uploads less than BASE
Flag BASE (94cee5b) HEAD (de77d6b)
13 5
Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #9930      +/-   ##
==========================================
- Coverage   93.08%   86.51%   -6.57%     
==========================================
  Files         187      187              
  Lines       15275    15279       +4     
  Branches      177      177              
==========================================
- Hits        14219    13219    -1000     
- Misses       1044     2043     +999     
- Partials       12       17       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mtrezza
Copy link
Member

mtrezza commented Nov 16, 2025

Adding metadata adds overhead and increase data transfer costs. Also, it's impossible to distinguish multiple servers as these IDs are static.

To address both, the metadata should be a Parse Server variable and only if the variable is set should the meta data be set for the driver. Also, is it possible to add a test?

@durran
Copy link
Author

durran commented Nov 18, 2025

Adding metadata adds overhead and increase data transfer costs. Also, it's impossible to distinguish multiple servers as these IDs are static.

To address both, the metadata should be a Parse Server variable and only if the variable is set should the meta data be set for the driver. Also, is it possible to add a test?

Sure I'll do that. We basically want handshakes sent to the server to contain metadata about the app that is using the driver so it appears in the server logs when the handshake completes. I'll add the variable and tests.

@alexbevi
Copy link

@mtrezza this is effectively the same change as meteor/meteor#13222. The main purpose is to help differentiate client connections in MongoDB's mongos/mongod logs, so that anyone reviewing their cluster's logs can trace connections more effectively back to the client that created it.

The added overhead here would be a handful of bytes, as some metadata is already being sent - this just adds a couple dozen additional bytes to that.

@mtrezza
Copy link
Member

mtrezza commented Nov 19, 2025

@alexbevi Someones use case should not have a mandatory cost implication for all others. We don't anticipate every possible use case to quantify the implication, nor do we guess someones resource pricing model, nor do we set an arbitrary pain limit for said implication.

For the same reason we strive for versatility. Allowing developers to set a custom value via an optional option caters to more use cases, e.g. identify server version, deployment, commit hash, etc. in MongoDB logs.

@alexbevi
Copy link

alexbevi commented Nov 20, 2025

@alexbevi Someones use case should not have a mandatory cost implication for all others. We don't anticipate every possible use case to quantify the implication, nor do we guess someones resource pricing model, nor do we set an arbitrary pain limit for said implication.

For the same reason we strive for versatility. Allowing developers to set a custom value via an optional option caters to more use cases, e.g. identify server version, deployment, commit hash, etc. in MongoDB logs.

I think that's reasonable to make additional information transmission configurable. Would a simple default of the adapter name only (ex: parse_server_gridfs or parse_server_storage) and making it opt-out be a better approach?

@mtrezza
Copy link
Member

mtrezza commented Nov 20, 2025

Opt-in makes more sense to me for the reasons I mentioned. I can't think of a reason to make this opt-out and use a default value. This is an optional log enhancement which is unnecessary in architectures where Parse Server is the only DB client, which I think is the majority, and likely unwanted in production deployments due to the unnecessary overhead. Data transfer can be a significant cost factor in MongoDB Atlas for example, so when thinking about scale, I lean towards opt-in. Even if just a few bytes, think of a scenario where minPoolSize and maxIdleTimeMS repeatedly keeps closing and opening connections, transmitting the additional log data. We strive for more efficiency while giving developers the option to be more verbose in logs, hence I think this PR with opt-in is a good suggestion.

Also, enriching the logs with server identifier and version by default exposes information that may provide an additional attack surface for lateral movements. The server version is currently only accessible with the master key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants