Skip to content

Commit

Permalink
Add logging infra to PineconeClient and Index.
Browse files Browse the repository at this point in the history
The idea is, each operation has a pair of log entries - start is logged at Trace level (and should be ignored in most cases), and then we either log success at Debug level, or error at Error level.
Only logging basic information like operation type and the index name - we want to avoid printing sensitive data in the logs for security reasons.

Also wired up logger(factory) to GrpcChannel and HttpClient for more detailed logging.

Fixes #118
  • Loading branch information
maumar committed Jul 24, 2024
1 parent 3eab568 commit 5371fed
Show file tree
Hide file tree
Showing 11 changed files with 388 additions and 91 deletions.
18 changes: 11 additions & 7 deletions src/Extensions.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using Microsoft.Extensions.Logging;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
Expand All @@ -8,24 +9,27 @@ internal static class Extensions
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static ValueTask CheckStatusCode(
this HttpResponseMessage response, CancellationToken ct, [CallerMemberName] string requestName = "")
this HttpResponseMessage response, ILogger logger, string operationName, CancellationToken ct, [CallerMemberName] string requestName = "")
{
#if NETSTANDARD2_0
return response.IsSuccessStatusCode ? default : ThrowOnFailedResponse(response, requestName, ct);
return response.IsSuccessStatusCode ? default : ThrowOnFailedResponse(response, requestName, logger, operationName, ct);
#else
return response.IsSuccessStatusCode ? ValueTask.CompletedTask : ThrowOnFailedResponse(response, requestName, ct);
return response.IsSuccessStatusCode ? ValueTask.CompletedTask : ThrowOnFailedResponse(response, requestName, logger, operationName, ct);
#endif

[DoesNotReturn, StackTraceHidden]
static async ValueTask ThrowOnFailedResponse(
HttpResponseMessage response, string requestName, CancellationToken ct)
HttpResponseMessage response, string requestName, ILogger logger, string operationName, CancellationToken ct)
{
throw new HttpRequestException($"{requestName} request has failed. " +
var message = $"{requestName} request has failed. " +
#if NETSTANDARD2_0
$"Code: {response.StatusCode}. Message: {await response.Content.ReadAsStringAsync().ConfigureAwait(false)}");
$"Code: {response.StatusCode}. Message: {await response.Content.ReadAsStringAsync().ConfigureAwait(false)}";
#else
$"Code: {response.StatusCode}. Message: {await response.Content.ReadAsStringAsync(ct).ConfigureAwait(false)}");
$"Code: {response.StatusCode}. Message: {await response.Content.ReadAsStringAsync(ct).ConfigureAwait(false)}";
#endif
logger.OperationFailed(operationName, message);

throw new HttpRequestException(message);
}
}

Expand Down
21 changes: 14 additions & 7 deletions src/Grpc/GrpcTransport.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using CommunityToolkit.Diagnostics;
using Grpc.Core;
using Grpc.Net.Client;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;

namespace Pinecone.Grpc;

Expand All @@ -12,17 +14,20 @@ namespace Pinecone.Grpc;

private readonly VectorService.VectorServiceClient Grpc;

public GrpcTransport(string host, string apiKey)
private readonly ILogger Logger;

public GrpcTransport(string host, string apiKey, ILoggerFactory? loggerFactory)
{
Guard.IsNotNullOrWhiteSpace(host);
Guard.IsNotNullOrWhiteSpace(apiKey);

Auth = new() { { Constants.GrpcApiKey, apiKey } };
Channel = GrpcChannel.ForAddress($"https://{host}");
Channel = GrpcChannel.ForAddress($"https://{host}", new GrpcChannelOptions { LoggerFactory = loggerFactory });
Grpc = new(Channel);
Logger = loggerFactory?.CreateLogger<GrpcTransport>() ?? (ILogger)NullLogger.Instance;
}

public static GrpcTransport Create(string host, string apiKey) => new(host, apiKey);
public static GrpcTransport Create(string host, string apiKey, ILoggerFactory? loggerFactory) => new(host, apiKey, loggerFactory);

public async Task<IndexStats> DescribeStats(MetadataMap? filter = null, CancellationToken ct = default)
{
Expand Down Expand Up @@ -68,8 +73,9 @@ public async Task<ScoredVector[]> Query(
}
else
{
ThrowHelper.ThrowArgumentException(
"At least one of the following parameters must be non-null: id, values, sparseValues");
var errorMessage = "At least one of the following parameters must be non-null: id, values, sparseValues.";
Logger.OperationFailed("Query", errorMessage);
ThrowHelper.ThrowArgumentException(errorMessage);
}

using var call = Grpc.QueryAsync(request, Auth, cancellationToken: ct);
Expand Down Expand Up @@ -113,8 +119,9 @@ public async Task Update(
{
if (values is null && sparseValues is null && metadata is null)
{
ThrowHelper.ThrowArgumentException(
"At least one of the following parameters must be non-null: values, sparseValues, metadata");
var errorMessage = "At least one of the following parameters must be non-null: values, sparseValues, metadata.";
Logger.OperationFailed("Update", errorMessage);
ThrowHelper.ThrowArgumentException(errorMessage);
}

var request = new UpdateRequest
Expand Down
5 changes: 3 additions & 2 deletions src/ITransport.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Diagnostics.CodeAnalysis;
using Microsoft.Extensions.Logging;
using Pinecone.Grpc;
using Pinecone.Rest;

Expand All @@ -12,9 +13,9 @@ public interface ITransport<
#endif
{
#if NET7_0_OR_GREATER
static abstract T Create(string host, string apiKey);
static abstract T Create(string host, string apiKey, ILoggerFactory? loggerFactory = null);
#elif NET6_0
static T Create(string host, string apiKey) => PineconeClient.CreateTransport<T>(host, apiKey);
static T Create(string host, string apiKey, ILoggerFactory? loggerFactory = null) => PineconeClient.CreateTransport<T>(host, apiKey, loggerFactory);
#endif

Task<IndexStats> DescribeStats(MetadataMap? filter = null, CancellationToken ct = default);
Expand Down
Loading

0 comments on commit 5371fed

Please sign in to comment.