Skip to content

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.

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
Merged
merged 40 commits into from
Aug 22, 2025
Merged

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

merged 40 commits into from
Aug 22, 2025

Conversation

blva
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.

Sign in to view

@blva @Copilot
Update src/helpers/deviceId.ts
72f1ab8 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
fmenezes
fmenezes reviewed Jul 31, 2025
View reviewed changes
fmenezes
fmenezes reviewed Jul 31, 2025
View reviewed changes
fmenezes
fmenezes reviewed Jul 31, 2025
View reviewed changes
fmenezes
fmenezes reviewed Jul 31, 2025
View reviewed changes
@blva blva requested a review from Copilot July 31, 2025 14:34
Copilot

This comment was marked as outdated.

Sign in to view

@blva blva requested a review from fmenezes July 31, 2025 15:34
fmenezes
fmenezes reviewed Jul 31, 2025
View reviewed changes
gagik
gagik reviewed Aug 1, 2025
View reviewed changes
gagik
gagik reviewed Aug 1, 2025
View reviewed changes
src/telemetry/telemetry.ts Outdated

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

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

gagik
gagik reviewed Aug 1, 2025
View reviewed changes
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

src/helpers/connectionOptions.ts Outdated
}

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!

nirinchev
nirinchev reviewed Aug 4, 2025
View reviewed changes
@blva blva requested a review from nirinchev August 19, 2025 11:46
gagik
gagik reviewed Aug 19, 2025
View reviewed changes
gagik
gagik reviewed Aug 21, 2025
View reviewed changes
src/helpers/deviceId.ts Outdated
}

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

src/telemetry/telemetry.ts
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?

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

gagik
gagik reviewed Aug 21, 2025
View reviewed changes
gagik
gagik approved these changes Aug 21, 2025
View reviewed changes
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.

gagik
gagik approved these changes Aug 22, 2025
View reviewed changes
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
@nirinchev
Merge branch 'main' into ni/http-tweaks
24097c3 
* 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.
4 participants
@blva @fmenezes @nirinchev @gagik