Skip to content

Add Aspire metadata to MongoClient#6938

Merged
eerhardt merged 2 commits intomicrosoft:mainfrom
adelinowona:libraryInfo-update
Dec 19, 2024
Merged

Add Aspire metadata to MongoClient#6938
eerhardt merged 2 commits intomicrosoft:mainfrom
adelinowona:libraryInfo-update

Conversation

@adelinowona
Copy link
Copy Markdown
Contributor

@adelinowona adelinowona commented Dec 12, 2024

Description

MongoDB drivers are used in various flavors and languages. Making sure we exercise our due diligence in identifying the "origin" of the library calls makes it best to understand how our Atlas servers get accessed.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
Microsoft Reviewers: Open in CodeFlow

@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Dec 12, 2024
Copy link
Copy Markdown
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, @adelinowona!

I just had one simple comment. Once that is addressed, I think this can be merged.

clientSettings.LoggingSettings ??= new LoggingSettings(serviceProvider.GetService<ILoggerFactory>());

var aspireVersion = typeof(MongoDBSettings).Assembly.GetName().Version?.ToString();
clientSettings.LibraryInfo = new LibraryInfo("aspire", aspireVersion);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you move this up to line 188, right after the clientSettings gets created?

The reasoning is that the app developer should be able to set this LibraryInfo in their configureClientSettings callback. Doing it here will overwrite anything the app developer tries configuring.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @eerhardt for raising this, we still want to record the aspire info so I have updated the code to append it if the user has already set the LibraryInfo.

@adelinowona adelinowona requested a review from eerhardt December 16, 2024 17:23
@adelinowona
Copy link
Copy Markdown
Contributor Author

@eerhardt, @sebastienros, @radical anyone has a chance to review this PR?

Copy link
Copy Markdown
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@eerhardt eerhardt merged commit 7f63747 into microsoft:main Dec 19, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 19, 2025
@github-actions github-actions Bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants