Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

feat: update connectionString appName param - [MCP-68] #406

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

Merged
blva merged 40 commits into main from MCP-68
Aug 22, 2025
Merged

feat: update connectionString appName param - [MCP-68] #406

blva merged 40 commits into main from MCP-68
Aug 22, 2025

Conversation

Copy link
Collaborator

@blva blva commented Jul 28, 2025 •
edited
Loading

Proposed changes

  • Refactored telemetry to use centralized device ID utility
  • Extended appName format to appName--deviceId--clientName
  • Created src/helpers/deviceId.ts utility for device ID retrieval
  • Updated setAppNameParamIfMissing function to support extended format
  • Modified Session and Atlas connect tools to use new format
  • Added unit tests and integration tests
  • Decoupled connectionString validation from connection

Checklist

@blva blva marked this pull request as ready for review July 31, 2025 13:13
@Copilot Copilot AI review requested due to automatic review settings July 31, 2025 13:13
@blva blva requested a review from a team as a code owner July 31, 2025 13:13
Copilot

This comment was marked as outdated. 

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@blva blva requested a review from Copilot July 31, 2025 14:34
Copilot

This comment was marked as outdated.

@blva blva requested a review from fmenezes July 31, 2025 15:34

private constructor(
private readonly session: Session,
private readonly userConfig: UserConfig,
private readonly commonProperties: CommonProperties,
{ eventCache, getRawMachineId }: { eventCache: EventCache; getRawMachineId: () => Promise<string> }
{ eventCache}: { eventCache: EventCache }
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think it'd still be good to pass getDeviceIdForConnection for explicit DI. It also helps us avoid mocking the file which is a bit of JavaScript magic

blva reacted with thumbs up emoji
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done! thanks

Copy link
Collaborator

@gagik gagik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It's important in my opinion to avoid double device ID resolution

}

const deviceId = components.deviceId || (await getDeviceIdForConnection());
Copy link
Collaborator

@gagik gagik Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

we shouldn't end up awaiting device ID a second time, can we organize it so this device ID gets passed from one point of entry and we never fallback to this function? deviceId doesn't get passed in connectToMongoDB right now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I refactored this quite a bit into a singleton so that we avoid double resolution, let me know what you think and if I'm missing any gaps!

@blva blva requested a review from nirinchev August 19, 2025 11:46
}

try {
const deviceId = await getDeviceId({
Copy link
Collaborator

Choose a reason for hiding this comment

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

getDeviceId already does plenty of error handling, including the abort error. I don't think this try/catch will ever catch anything

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

does it automatically assign unknown upon error? If so we can remove

this.commonProperties.is_container_env = containerEnv;

this.isBufferingEvents = false;
}

public async close(): Promise<void> {
this.deviceIdAbortController.abort();
this.isBufferingEvents = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

would this.deviceId.close make sense here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved it to index.ts so I'm closing it there as well, makes sense?

gagik reacted with thumbs up emoji
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

now moved to transport

Copy link
Collaborator

@gagik gagik left a comment

Choose a reason for hiding this comment

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

Other than above, changes look good to me 🙂 feel free to go ahead as you see fit.

Copy link
Collaborator

@gagik gagik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great 🚀 ,(削除) just one comment about some remainders of static-ness (削除ここまで) actually that works as expected

@blva blva enabled auto-merge (squash) August 22, 2025 11:55
@blva blva disabled auto-merge August 22, 2025 11:55
@blva blva merged commit ca68195 into main Aug 22, 2025
20 of 22 checks passed
@blva blva deleted the MCP-68 branch August 22, 2025 12:04
nirinchev added a commit that referenced this pull request Aug 22, 2025 
* main:
 feat: update connectionString appName param - [MCP-68] (#406)
 chore: remove double occurrence of an object's property (#469)
 chore: add user for the smithery dockerfile (#458)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmenezes fmenezes fmenezes left review comments

@gagik gagik gagik approved these changes

Copilot code review Copilot Copilot left review comments

@nirinchev nirinchev Awaiting requested review from nirinchev

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

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /