Skip to content

Fix OpenAI connection-pool-metrics-enabled being ignored - #6802

Open
wantaekchoi wants to merge 1 commit into
spring-projects:mainfrom
wantaekchoi:fix-openai-connection-pool-metrics
Open

Fix OpenAI connection-pool-metrics-enabled being ignored#6802
wantaekchoi wants to merge 1 commit into
spring-projects:mainfrom
wantaekchoi:fix-openai-connection-pool-metrics

Conversation

@wantaekchoi

Copy link
Copy Markdown

Problem

OpenAiAutoConfigurationUtil.resolveCommonProperties builds the ResolvedConnectionProperties instance that every OpenAI model auto-configuration reads. It resolves each connection property from the model-level or the common-level properties, but there is no branch for connectionPoolMetricsEnabled, so the resolved value stays at its false default whatever the user configures.

All six OpenAI auto-configurations (chat, embedding, image, moderation, audio speech, audio transcription) pick their meter registry from that resolved value:

MeterRegistry meterRegistryToUse = commonProperties.isConnectionPoolMetricsEnabled()
        ? meterRegistry.getIfAvailable() : null;

The parameter is named commonProperties, but each caller passes resolvedProperties into it. So all six pass null to the OpenAI clients, and SpringAiOpenAiHttpClient binds OkHttpConnectionPoolMetrics only when the registry is non-null.

The opt-in that openai-chat.adoc tells users to set therefore does nothing:

spring.ai.openai.chat.connection-pool-metrics-enabled=true

Neither does spring.ai.openai.connection-pool-metrics-enabled, the row listed in the property table of all six OpenAI reference pages. Auto-configuration never registers an okhttp.pool.* gauge.

Commit 386df9c added the property and the six consumers but did not touch OpenAiAutoConfigurationUtil.

Changes

Resolve connectionPoolMetricsEnabled next to the sibling booleans, with the resolution they already use:

resolved.setConnectionPoolMetricsEnabled(
        modelProperties.isConnectionPoolMetricsEnabled() || commonProperties.isConnectionPoolMetricsEnabled());

For a primitive boolean defaulting to false, that OR is the same rule the other properties follow: take the model value unless it is the default, otherwise take the common value. Both documented keys start working, and the property stays opt-in.

It does not let a model-level false override a common-level true. Distinguishing "explicitly false" from "not set" needs a nullable Boolean, and isMicrosoftFoundry and isGitHubModels do not do that either.

I checked the rest of the resolver rather than this field alone. Comparing every setter on AbstractOpenAiProperties against the ones resolveCommonProperties assigns, connectionPoolMetricsEnabled was the only property left unresolved.

Testing

OpenAiChatPropertiesTests.connectionPoolMetricsEnabled starts the chat auto-configuration with spring.ai.openai.connection-pool-metrics-enabled=true and a SimpleMeterRegistry, then asserts that okhttp.pool.connection.count gauges are registered. Without the change it fails with Expecting actual not to be empty, since no gauge is bound. connectionPoolMetricsDisabledByDefault pins the opt-in default.

OpenAiAutoConfigurationUtilTests adds three resolver cases: enabled at the common level only, enabled at the model level only, and enabled at neither. The first two fail without the change.

./mvnw -Dmaven.build.cache.enabled=false -pl auto-configurations/models/spring-ai-autoconfigure-model-openai test

passes with 29 tests. starters/spring-ai-starter-model-openai and auto-configurations/models/spring-ai-autoconfigure-model-mistral-ai, the modules that depend on it, pass with 12 tests.

./mvnw -Dmaven.build.cache.enabled=false clean package -DskipITs also passes, with 5662 tests and no failures.

`OpenAiAutoConfigurationUtil.resolveCommonProperties` builds the
`ResolvedConnectionProperties` that all six OpenAI model
auto-configurations read, but it never copies
`connectionPoolMetricsEnabled` from either the model-level or the
common-level properties. The resolved value stays at its `false`
default, the auto-configurations hand a null `MeterRegistry` to the
OpenAI clients, and `OkHttpConnectionPoolMetrics` is never bound, so
the documented opt-in has no effect.

Propagate the flag next to the sibling booleans, using the same
model-or-common resolution they use.

Signed-off-by: wantaek <wantaekchoi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants