Skip to content
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

Add health check support for Qdrant client #6057

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

Alirexaa
Copy link
Contributor

@Alirexaa Alirexaa commented Oct 1, 2024

Description

This PR Adds health check support for Aspire.Qdrant.Client.

We need to import Testcontainers for ConformanceTests.

Fixes #5768

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?
    • Yes
      • Link to aspire-docs issue:
    • No
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 Oct 1, 2024
@radical
Copy link
Member

radical commented Oct 3, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

src/Aspire.Hosting/Aspire.Hosting.csproj Outdated Show resolved Hide resolved
tests/Aspire.Qdrant.Client.Tests/QdrantContainerFixture.cs Outdated Show resolved Hide resolved
tests/Aspire.Qdrant.Client.Tests/ConformanceTests.cs Outdated Show resolved Hide resolved
Comment on lines 87 to 88
// use http client for register health check
var httpClientName = serviceKey is null ? "qdrant-healthchecks" : $"qdrant-healthchecks_{connectionName}";
Copy link
Member

Choose a reason for hiding this comment

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

Why not implement this in Xabaril/AspNetCore.Diagnostics.HealthChecks#2186? Then everyone can use it, and not just people using the Aspire integration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

qdrant-client added health check method via qdrant/qdrant-dotnet#71. the latest version should have this method. I will try to use the client method instead of the current implementation in Aspire. also, I will update Xabaril PR to use this new health check method and we will update the existing aspire client to use the Xabaril package when the new health check package is released.

NuGet.config Outdated Show resolved Hide resolved
@Alirexaa
Copy link
Contributor Author

I still have this problem #6432 (comment)
@eerhardt can you help me fix that?

@eerhardt
Copy link
Member

I still have this problem #6432 (comment) @eerhardt can you help me fix that?

From the root of your repo, run the following and ensure you get the same output:

❯ dotnet nuget list source
Registered Sources:
  1.  dotnet-public [Enabled]
      https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json
  2.  dotnet-eng [Enabled]
      https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json
  3.  dotnet9 [Enabled]
      https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9/nuget/v3/index.json
  4.  dotnet-libraries [Enabled]
      https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-libraries/nuget/v3/index.json
  5.  dotnet9-transport [Enabled]
      https://dnceng.pkgs.visualstudio.com/public/_packaging/dotnet9-transport/nuget/v3/index.json

If any of those are disabled, make sure you enable them, usually in VS: Tools => NuGet Package Manager => Package Manager Settings => Package Sources.

@Alirexaa
Copy link
Contributor Author

I still have this problem #6432 (comment) @eerhardt can you help me fix that?

From the root of your repo, run the following and ensure you get the same output:

❯ dotnet nuget list source
Registered Sources:
  1.  dotnet-public [Enabled]
      https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json
  2.  dotnet-eng [Enabled]
      https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json
  3.  dotnet9 [Enabled]
      https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9/nuget/v3/index.json
  4.  dotnet-libraries [Enabled]
      https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-libraries/nuget/v3/index.json
  5.  dotnet9-transport [Enabled]
      https://dnceng.pkgs.visualstudio.com/public/_packaging/dotnet9-transport/nuget/v3/index.json

If any of those are disabled, make sure you enable them, usually in VS: Tools => NuGet Package Manager => Package Manager Settings => Package Sources.

Thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Qdrant client doesn't have HealthChecks
3 participants