Add internal text embedding subsystem for embedding text#3104
Add internal text embedding subsystem for embedding text#3104
Conversation
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
…configuration Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
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 <1749983+JerryNixon@users.noreply.github.com>
… and telemetry integration Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
... Addressed in commit 89cb2d9:
|
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
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 <1749983+JerryNixon@users.noreply.github.com>
... 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 <1749983+JerryNixon@users.noreply.github.com>
…orization Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
... Addressed in commits 3e02c0f and 5c05464. Health Check Implementation was already complete:
Endpoint Implementation is now complete:
Schema already includes |
|
@JerryNixon, should we add a config validation to RuntimeConfigValidator? |
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.