-
Notifications
You must be signed in to change notification settings - Fork 304
Add internal text embedding subsystem for embedding text #3104
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: JerryNixon <[email protected]>
Co-authored-by: JerryNixon <[email protected]>
…configuration Co-authored-by: JerryNixon <[email protected]>
Co-authored-by: JerryNixon <[email protected]>
Co-authored-by: JerryNixon <[email protected]>
Co-authored-by: JerryNixon <[email protected]>
Co-authored-by: JerryNixon <[email protected]>
Co-authored-by: JerryNixon <[email protected]>
Architecture & Design Suggestions1. Configuration Property Naming InconsistencyThe JSON schema uses // Current: Inconsistent naming
[JsonPropertyName("endpoint")]
public string BaseUrl { get; init; }
// Suggestion: Align property name with JSON
[JsonPropertyName("endpoint")]
public string Endpoint { get; init; }2. Missing Validation in EmbeddingService ConstructorThe service should validate configuration on startup: public EmbeddingService(...)
{
_httpClient = httpClient ?? throw new ArgumentNullException(nameof(httpClient));
_options = options ?? throw new ArgumentNullException(nameof(options));
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
_cache = cache ?? throw new ArgumentNullException(nameof(cache));
// Add validation
if (_options.Provider == EmbeddingProviderType.AzureOpenAI && string.IsNullOrEmpty(_options.EffectiveModel))
{
throw new InvalidOperationException("Model/deployment name is required for Azure OpenAI provider.");
}
if (string.IsNullOrEmpty(_options.BaseUrl))
{
throw new InvalidOperationException("Base URL is required for embedding service.");
}
ConfigureHttpClient();
}3. Cache Key Security ConcernUsing SHA256 for cache keys is good, but consider adding provider/model to the cache key to prevent collisions if configuration changes: private string CreateCacheKey(string text)
{
// Include provider and model in hash to avoid cross-provider collisions
string keyInput = $"{_options.Provider}:{_options.EffectiveModel}:{text}";
byte[] textBytes = Encoding.UTF8.GetBytes(keyInput);
byte[] hashBytes = SHA256.HashData(textBytes);
string hashHex = Convert.ToHexString(hashBytes);
return $"{CACHE_KEY_PREFIX}{KEY_DELIMITER}{hashHex}";
}4. Missing Telemetry IntegrationThe public async Task<float[]> EmbedAsync(string text, CancellationToken cancellationToken = default)
{
using Activity? activity = EmbeddingTelemetryHelper.StartEmbeddingActivity(nameof(EmbedAsync));
activity?.SetEmbeddingActivityTags(_options.Provider.ToString(), _options.EffectiveModel, 1);
Stopwatch sw = Stopwatch.StartNew();
try
{
EmbeddingTelemetryHelper.TrackEmbeddingRequest(_options.Provider.ToString(), 1);
// ... existing implementation ...
sw.Stop();
EmbeddingTelemetryHelper.TrackTotalDuration(_options.Provider.ToString(), sw.Elapsed, fromCache);
activity?.SetEmbeddingActivitySuccess(sw.Elapsed.TotalMilliseconds, embedding.Length);
return embedding;
}
catch (Exception ex)
{
sw.Stop();
EmbeddingTelemetryHelper.TrackError(_options.Provider.ToString(), ex.GetType().Name);
activity?.SetEmbeddingActivityError(ex);
throw;
}
}5. Hot Reload Support Not ImplementedYou registered // In ConfigureServices after registering the service
if (runtimeConfigAvailable && runtimeConfig?.Runtime?.IsEmbeddingsConfigured == true)
{
// ... existing registration ...
// Register hot reload handler
_hotReloadEventPublisher.Subscribe(
DabConfigEvents.EMBEDDING_SERVICE_ON_CONFIG_CHANGED,
async (newConfig) =>
{
if (newConfig is RuntimeConfig rc && rc.Runtime?.Embeddings != null)
{
_logger.LogInformation("Reloading embedding service configuration");
// Recreate service with new config or update existing instance
}
});
}6. Missing Health Check ImplementationThe public class EmbeddingHealthCheck : IHealthCheck
{
private readonly IEmbeddingService _embeddingService;
private readonly EmbeddingsHealthCheckConfig _config;
public async Task<HealthCheckResult> CheckHealthAsync(
HealthCheckContext context,
CancellationToken cancellationToken = default)
{
if (!_embeddingService.IsEnabled)
return HealthCheckResult.Healthy("Embedding service is disabled");
var sw = Stopwatch.StartNew();
var result = await _embeddingService.TryEmbedAsync(_config.TestText, cancellationToken);
sw.Stop();
if (!result.Success)
return HealthCheckResult.Unhealthy($"Embedding failed: {result.ErrorMessage}");
if (sw.ElapsedMilliseconds >= _config.ThresholdMs)
return HealthCheckResult.Degraded($"Response time {sw.ElapsedMilliseconds}ms exceeds threshold {_config.ThresholdMs}ms");
if (_config.ExpectedDimensions.HasValue && result.Embedding?.Length != _config.ExpectedDimensions)
return HealthCheckResult.Unhealthy($"Expected {_config.ExpectedDimensions} dimensions, got {result.Embedding?.Length}");
return HealthCheckResult.Healthy($"Embedding service healthy ({sw.ElapsedMilliseconds}ms)");
}
}7. Missing Endpoint ImplementationThe [ApiController]
[Route("[controller]")]
public class EmbeddingController : ControllerBase
{
private readonly IEmbeddingService _embeddingService;
private readonly EmbeddingsOptions _options;
[HttpPost]
public async Task<IActionResult> Embed([FromBody] EmbedRequest request)
{
if (!_embeddingService.IsEnabled)
return StatusCode(503, "Embedding service is disabled");
if (request.Input is string text)
{
var result = await _embeddingService.TryEmbedAsync(text);
return result.Success ? Ok(new { embedding = result.Embedding }) : BadRequest(result.ErrorMessage);
}
else if (request.Input is string[] texts)
{
var result = await _embeddingService.TryEmbedBatchAsync(texts);
return result.Success ? Ok(new { embeddings = result.Embeddings }) : BadRequest(result.ErrorMessage);
}
return BadRequest("Invalid input format");
}
}8. Test Coverage GapsThe tests only cover configuration - add integration tests: [TestMethod]
public async Task EmbedAsync_WithMockHttpClient_ReturnsValidEmbedding()
{
// Mock HTTP response
var mockHandler = new Mock<HttpMessageHandler>();
mockHandler.Protected()
.Setup<Task<HttpResponseMessage>>("SendAsync", ...)
.ReturnsAsync(new HttpResponseMessage
{
StatusCode = HttpStatusCode.OK,
Content = new StringContent(@"{
""data"": [{
""index"": 0,
""embedding"": [0.1, 0.2, 0.3]
}]
}")
});
var httpClient = new HttpClient(mockHandler.Object);
var service = new EmbeddingService(httpClient, options, logger, cache);
var result = await service.EmbedAsync("test");
Assert.IsNotNull(result);
Assert.AreEqual(3, result.Length);
}9. Error Handling for Rate LimitingAdd retry logic for transient failures: // Consider using Polly for retry policies
private static readonly AsyncRetryPolicy _retryPolicy = Policy
.Handle<HttpRequestException>()
.Or<TaskCanceledException>()
.WaitAndRetryAsync(3, retryAttempt =>
TimeSpan.FromSeconds(Math.Pow(2, retryAttempt)));10. Documentation Gaps
In addition the added "test-text" parameter in health check is unnecessary. Just use "health check" and do not ask for a default value. Same with expected dimensions. That should not be in the configuration as it can be inferred. |
Co-authored-by: JerryNixon <[email protected]>
… and telemetry integration Co-authored-by: JerryNixon <[email protected]>
... Addressed in commit 89cb2d9:
|
Co-authored-by: JerryNixon <[email protected]>
Code Review: Issues Found1. Schema mismatch between JSON schema and codeFile: The schema uses "endpoint": {
"type": "string",
"description": "The provider base URL endpoint..."
}But the converter expects case "base-url":
baseUrl = JsonSerializer.Deserialize<string>(ref reader, options);
break;2. Unused field in
|
…ate empty embeddings Co-authored-by: JerryNixon <[email protected]>
... Addressed in commit e8d7238:
|
Health Check ReviewWhat's Missing/Incomplete
|
| Component | Configuration | Implementation | Complete? |
|---|---|---|---|
| Health Check Config | ✅ Complete | ❌ Not Implemented | No |
| Health Check Execution | N/A | ❌ Missing | No |
| Endpoint Auth Config | ✅ Complete | ❌ Not Implemented | No |
| Endpoint Controller | N/A | ❌ Missing | No |
| Role Enforcement | ✅ Helper methods exist | ❌ Not wired up | No |
Recommendations
-
For Health Check completeness:
- Implement
IHealthCheckfor embeddings (e.g.,EmbeddingHealthCheck.cs) - Register in
Startup.cswithservices.AddHealthChecks().AddCheck<EmbeddingHealthCheck>("embedding") - Use the
EmbeddingService.TryEmbedAsync()withHealth.TestText - Compare response time to
Health.ThresholdMs - Optionally validate
ExpectedDimensions
- Implement
-
For Auth completeness:
- Implement an
/embedendpoint - Add authorization middleware that uses
EmbeddingsEndpointOptions.IsRoleAllowed() - Update the JSON schema to include the
endpointsub-object
- Implement an
…oint/health sub-objects Co-authored-by: JerryNixon <[email protected]>
…orization Co-authored-by: JerryNixon <[email protected]>
Co-authored-by: JerryNixon <[email protected]>
... Addressed in commits 3e02c0f and 5c05464. Health Check Implementation was already complete:
Endpoint Implementation is now complete:
Schema already includes |
Why make this change?
Internal DAB system for text embedding/vectorization to support future parameter substitution and Redis semantic search features.
What is this change?
Configuration (
runtime.embeddings)enabled: Master toggle (default:true)provider:azure-openai|openaibase-url,api-key,model: Provider connection (supports@env())api-version,dimensions,timeout-ms: Optional tuningendpoint.enabled/path/roles: Optional REST endpoint at configured path (default:/embed)health.enabled/threshold-ms/test-text/expected-dimensions: Health check configCore Components
IEmbeddingServicewithTryEmbedAsync()pattern - returns result objects, no exceptionsEmbeddingService- HTTP client with FusionCache L1 (24h TTL, SHA256 hash keys with provider/model included)EmbeddingsOptionsConverterFactory- Custom JSON deserializer with env var replacementEmbeddingTelemetryHelper- OpenTelemetry metrics/spans for latency, cache hits, dimensionsEmbeddingController- REST endpoint for/embedwith role-based authorizationHealth Check Implementation ✅
HealthCheckHelper.UpdateEmbeddingsHealthCheckResultsAsync()- Executes test embedding with threshold validationhealth.threshold-mshealth.expected-dimensionsif specifiedConfigurationDetailsincludesEmbeddingsandEmbeddingsEndpointstatusREST Endpoint Implementation ✅
EmbeddingControllerserves POST requests at configured path (default:/embed)X-MS-API-ROLEheaderValidation & Safety
Telemetry Integration
TryEmbedAsyncandTryEmbedBatchAsyncinstrumented with activity spans and metricsIntegration Points
embeddingsstatus in comprehensive checksEMBEDDINGS_CONFIG_CHANGEDeventdab configure --runtime.embeddings.*Code Organization
Azure.DataApiBuilder.Config.ObjectModel.Embeddings- Config modelsAzure.DataApiBuilder.Core.Services.Embeddings- Service, telemetry, interfaceAzure.DataApiBuilder.Service.Controllers- EmbeddingControllerHow was this tested?
17 unit tests covering deserialization, serialization, TryEmbed pattern, enable/disable behavior, env var replacement.
Sample Request(s)
Configuration:
{ "runtime": { "embeddings": { "enabled": true, "provider": "azure-openai", "base-url": "@env('EMBEDDINGS_ENDPOINT')", "api-key": "@env('EMBEDDINGS_API_KEY')", "model": "text-embedding-ada-002", "endpoint": { "enabled": true, "path": "/embed", "roles": ["authenticated"] }, "health": { "enabled": true, "threshold-ms": 5000, "test-text": "health check" } } } }Embed Endpoint:
Response:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.