diff --git a/apps/api/src/main/resources/application.yml b/apps/api/src/main/resources/application.yml index 74601d8b..da652bb3 100644 --- a/apps/api/src/main/resources/application.yml +++ b/apps/api/src/main/resources/application.yml @@ -179,3 +179,19 @@ management: web: exposure: include: health,info,modulith + +logging: + level: + # The provider libraries concatenate the prompt and the response content into + # WARN messages of their own, which `spring.ai.chat.observations.log-prompt` + # does not reach because it only governs Spring AI observation handlers. + # Every such site is guarded by isWarnEnabled(), so pinning these packages + # above WARN stops the payload from being built at all. + # + # This is the safe default, not the enforcement: LOGGING_LEVEL_* environment + # variables, system properties and logback configuration all outrank it. + # ProviderLoggingBoundaryVerifier checks the resolved level at startup and + # refuses to run with these open. + # See docs/runbooks/graph-rag-production-hardening.md. + org.springframework.ai.openai: ERROR + org.springframework.ai.anthropic: ERROR diff --git a/apps/api/src/test/java/com/orgmemory/api/observability/ProviderLoggingBoundaryTests.java b/apps/api/src/test/java/com/orgmemory/api/observability/ProviderLoggingBoundaryTests.java new file mode 100644 index 00000000..f69ad500 --- /dev/null +++ b/apps/api/src/test/java/com/orgmemory/api/observability/ProviderLoggingBoundaryTests.java @@ -0,0 +1,105 @@ +package com.orgmemory.api.observability; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +import java.io.File; +import java.io.IOException; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import org.junit.jupiter.api.Test; +import org.springframework.boot.env.YamlPropertySourceLoader; +import org.springframework.core.env.PropertySource; +import org.springframework.core.io.ClassPathResource; +import org.springframework.core.io.FileSystemResource; + +/** + * The OpenAI and Anthropic client libraries build WARN messages that concatenate + * the prompt and the response content. {@code spring.ai.chat.observations.log-prompt} + * does not reach them because it only governs Spring AI observation handlers. + * + *

Every such call site is guarded by {@code isWarnEnabled()}, so the level decides + * whether the payload is built at all. This test covers the shipped default only. + * The default is not the enforcement — environment variables and system properties + * outrank it — which is what {@code ProviderLoggingBoundaryVerifier} checks at startup + * against the resolved level. This test stops the default from drifting. + */ +class ProviderLoggingBoundaryTests { + + /** Packages whose own logging concatenates OrgMemory prompt or completion text. */ + private static final List PAYLOAD_LOGGING_PACKAGES = List.of( + "org.springframework.ai.openai", + "org.springframework.ai.anthropic"); + + /** Levels at or above ERROR; anything else lets a guarded WARN site build its message. */ + private static final Set LEVELS_ABOVE_WARN = Set.of("ERROR", "FATAL", "OFF"); + + @Test + void theBaseConfigurationPinsEveryPayloadLoggingPackageAboveWarn() throws IOException { + Map properties = load(new ClassPathResource("application.yml")); + + for (String payloadPackage : PAYLOAD_LOGGING_PACKAGES) { + // The loader returns origin-tracked char sequences rather than plain strings. + Object level = properties.get("logging.level." + payloadPackage); + assertEquals( + "ERROR", + String.valueOf(level), + () -> payloadPackage + + " must be pinned to ERROR in application.yml so its guarded WARN" + + " sites cannot concatenate the prompt"); + } + } + + @Test + void noProfileLowersAPayloadLoggingPackageBackToWarn() throws IOException { + File[] profiles = profileConfigurationFiles(); + assertTrue(profiles.length > 0, "no application-.yml was scanned, so this test proves nothing"); + + for (File profile : profiles) { + Map properties = load(new FileSystemResource(profile)); + + properties.forEach((key, value) -> { + if (!key.startsWith("logging.level.")) { + return; + } + String logger = key.substring("logging.level.".length()); + // Ancestors and descendants both matter: a parent pinned above the packages + // relaxes them, and a child such as org.springframework.ai.openai.api overrides + // the parent pin for everything under it. + if (PAYLOAD_LOGGING_PACKAGES.stream().noneMatch( + p -> p.equals(logger) || p.startsWith(logger + ".") || logger.startsWith(p + "."))) { + return; + } + assertTrue( + LEVELS_ABOVE_WARN.contains(String.valueOf(value).toUpperCase(Locale.ROOT)), + () -> profile.getName() + " sets " + key + "=" + value + + ", which re-enables the provider WARN sites that log the prompt"); + }); + } + } + + private static Map load(org.springframework.core.io.Resource resource) throws IOException { + List> sources = new YamlPropertySourceLoader().load(resource.getFilename(), resource); + if (sources.size() != 1) { + fail(resource.getFilename() + " must contain exactly one YAML document, found " + sources.size()); + } + @SuppressWarnings("unchecked") + Map properties = (Map) sources.getFirst().getSource(); + return properties; + } + + /** + * Profile files live beside {@code application.yml} in this module's resources. Gradle + * extracts them to a directory, so listing the parent is enough and keeps the test from + * hard-coding a profile list that a new profile would silently escape. + */ + private static File[] profileConfigurationFiles() throws IOException { + File resources = new ClassPathResource("application.yml").getFile().getParentFile(); + File[] profiles = resources.listFiles((directory, name) -> name.startsWith("application-") + && (name.endsWith(".yml") || name.endsWith(".yaml"))); + return profiles == null ? new File[0] : profiles; + } +} diff --git a/apps/worker/src/main/java/com/orgmemory/worker/graph/GraphIndexingProcessor.java b/apps/worker/src/main/java/com/orgmemory/worker/graph/GraphIndexingProcessor.java index 5a3b4c4a..1c89412f 100644 --- a/apps/worker/src/main/java/com/orgmemory/worker/graph/GraphIndexingProcessor.java +++ b/apps/worker/src/main/java/com/orgmemory/worker/graph/GraphIndexingProcessor.java @@ -79,7 +79,8 @@ class GraphIndexingProcessor { embeddingModels, routes, properties, - GraphRagEventSink.composite(eventSinks.orderedStream().toList())); + GraphRagEventSink.failureTolerant( + GraphRagEventSink.composite(eventSinks.orderedStream().toList()))); } GraphIndexingProcessor( diff --git a/apps/worker/src/main/resources/application.yml b/apps/worker/src/main/resources/application.yml index 8d973183..886b6ea8 100644 --- a/apps/worker/src/main/resources/application.yml +++ b/apps/worker/src/main/resources/application.yml @@ -114,3 +114,19 @@ management: tracing: sampling: probability: ${ORGMEMORY_TRACING_SAMPLING_PROBABILITY:0.1} + +logging: + level: + # The provider libraries concatenate the prompt and the response content into + # WARN messages of their own, which `spring.ai.chat.observations.log-prompt` + # does not reach because it only governs Spring AI observation handlers. + # Every such site is guarded by isWarnEnabled(), so pinning these packages + # above WARN stops the payload from being built at all. + # + # This is the safe default, not the enforcement: LOGGING_LEVEL_* environment + # variables, system properties and logback configuration all outrank it. + # ProviderLoggingBoundaryVerifier checks the resolved level at startup and + # refuses to run with these open. + # See docs/runbooks/graph-rag-production-hardening.md. + org.springframework.ai.openai: ERROR + org.springframework.ai.anthropic: ERROR diff --git a/apps/worker/src/test/java/com/orgmemory/worker/observability/ProviderLoggingBoundaryTests.java b/apps/worker/src/test/java/com/orgmemory/worker/observability/ProviderLoggingBoundaryTests.java new file mode 100644 index 00000000..b37ce494 --- /dev/null +++ b/apps/worker/src/test/java/com/orgmemory/worker/observability/ProviderLoggingBoundaryTests.java @@ -0,0 +1,105 @@ +package com.orgmemory.worker.observability; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +import java.io.File; +import java.io.IOException; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import org.junit.jupiter.api.Test; +import org.springframework.boot.env.YamlPropertySourceLoader; +import org.springframework.core.env.PropertySource; +import org.springframework.core.io.ClassPathResource; +import org.springframework.core.io.FileSystemResource; + +/** + * The OpenAI and Anthropic client libraries build WARN messages that concatenate + * the prompt and the response content. {@code spring.ai.chat.observations.log-prompt} + * does not reach them because it only governs Spring AI observation handlers. + * + *

Every such call site is guarded by {@code isWarnEnabled()}, so the level decides + * whether the payload is built at all. The worker runs graph extraction over chunk + * content, so its prompts carry evidence text directly. This test covers the shipped + * default only; {@code ProviderLoggingBoundaryVerifier} enforces the resolved level at + * startup, which is where an environment override is caught. + */ +class ProviderLoggingBoundaryTests { + + /** Packages whose own logging concatenates OrgMemory prompt or completion text. */ + private static final List PAYLOAD_LOGGING_PACKAGES = List.of( + "org.springframework.ai.openai", + "org.springframework.ai.anthropic"); + + /** Levels at or above ERROR; anything else lets a guarded WARN site build its message. */ + private static final Set LEVELS_ABOVE_WARN = Set.of("ERROR", "FATAL", "OFF"); + + @Test + void theBaseConfigurationPinsEveryPayloadLoggingPackageAboveWarn() throws IOException { + Map properties = load(new ClassPathResource("application.yml")); + + for (String payloadPackage : PAYLOAD_LOGGING_PACKAGES) { + // The loader returns origin-tracked char sequences rather than plain strings. + Object level = properties.get("logging.level." + payloadPackage); + assertEquals( + "ERROR", + String.valueOf(level), + () -> payloadPackage + + " must be pinned to ERROR in application.yml so its guarded WARN" + + " sites cannot concatenate the prompt"); + } + } + + @Test + void noProfileLowersAPayloadLoggingPackageBackToWarn() throws IOException { + File[] profiles = profileConfigurationFiles(); + assertTrue(profiles.length > 0, "no application-.yml was scanned, so this test proves nothing"); + + for (File profile : profiles) { + Map properties = load(new FileSystemResource(profile)); + + properties.forEach((key, value) -> { + if (!key.startsWith("logging.level.")) { + return; + } + String logger = key.substring("logging.level.".length()); + // Ancestors and descendants both matter: a parent pinned above the packages + // relaxes them, and a child such as org.springframework.ai.openai.api overrides + // the parent pin for everything under it. + if (PAYLOAD_LOGGING_PACKAGES.stream().noneMatch( + p -> p.equals(logger) || p.startsWith(logger + ".") || logger.startsWith(p + "."))) { + return; + } + assertTrue( + LEVELS_ABOVE_WARN.contains(String.valueOf(value).toUpperCase(Locale.ROOT)), + () -> profile.getName() + " sets " + key + "=" + value + + ", which re-enables the provider WARN sites that log the prompt"); + }); + } + } + + private static Map load(org.springframework.core.io.Resource resource) throws IOException { + List> sources = new YamlPropertySourceLoader().load(resource.getFilename(), resource); + if (sources.size() != 1) { + fail(resource.getFilename() + " must contain exactly one YAML document, found " + sources.size()); + } + @SuppressWarnings("unchecked") + Map properties = (Map) sources.getFirst().getSource(); + return properties; + } + + /** + * Profile files live beside {@code application.yml} in this module's resources. Gradle + * extracts them to a directory, so listing the parent is enough and keeps the test from + * hard-coding a profile list that a new profile would silently escape. + */ + private static File[] profileConfigurationFiles() throws IOException { + File resources = new ClassPathResource("application.yml").getFile().getParentFile(); + File[] profiles = resources.listFiles((directory, name) -> name.startsWith("application-") + && (name.endsWith(".yml") || name.endsWith(".yaml"))); + return profiles == null ? new File[0] : profiles; + } +} diff --git a/components/graph-rag-core/src/main/java/com/orgmemory/graphrag/observability/FailureTolerantGraphRagEventSink.java b/components/graph-rag-core/src/main/java/com/orgmemory/graphrag/observability/FailureTolerantGraphRagEventSink.java new file mode 100644 index 00000000..03aec555 --- /dev/null +++ b/components/graph-rag-core/src/main/java/com/orgmemory/graphrag/observability/FailureTolerantGraphRagEventSink.java @@ -0,0 +1,96 @@ +package com.orgmemory.graphrag.observability; + +import java.lang.System.Logger; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; + +/** + * Absorbs a failing telemetry backend without letting the failure disappear. + * + *

Producers already treat emission as non-critical and catch whatever the sink + * throws, which is correct — an observability backend must never decide whether a + * retrieval or an indexing job succeeds. The cost is that a sink broken since + * startup looks exactly like a sink with nothing to report. This wrapper keeps the + * absorption and adds the signal that was missing: a running count, the type of the + * most recent failure, and one log line the first time each kind of failure appears. + * + *

Only class names are recorded. A telemetry backend's exception message can + * quote the request it failed to send, so the message and the stack trace are + * treated the same way as any other payload and never leave this class. + * + *

Logging goes through {@link System.Logger} so that + * {@code components/graph-rag-core} keeps its property of having no runtime + * dependencies; Spring Boot's JUL bridge routes it into the application log. + */ +public final class FailureTolerantGraphRagEventSink implements GraphRagEventSink { + + private static final Logger LOGGER = + System.getLogger(FailureTolerantGraphRagEventSink.class.getName()); + + /** + * How many distinct failure types are worth a log line. A backend that produces more + * than this is malfunctioning in a way one more line will not clarify. + */ + private static final int REPORTED_FAILURE_TYPE_LIMIT = 10; + + private final GraphRagEventSink delegate; + private final AtomicLong swallowedFailures = new AtomicLong(); + private final AtomicReference lastFailureType = new AtomicReference<>(); + private final Set reportedFailureTypes = ConcurrentHashMap.newKeySet(); + + FailureTolerantGraphRagEventSink(GraphRagEventSink delegate) { + this.delegate = Objects.requireNonNull(delegate, "delegate"); + } + + @Override + public void emit(GraphRagEvent event) { + try { + delegate.emit(event); + } catch (RuntimeException failure) { + record(failure); + } + } + + /** How many events this sink has dropped since startup. Never resets. */ + public long swallowedFailureCount() { + return swallowedFailures.get(); + } + + /** Class name of the most recent failure, or {@code null} while nothing has failed. */ + public String lastFailureType() { + return lastFailureType.get(); + } + + /** How many log lines this sink has produced. One per distinct failure kind. */ + int reportedFailureTypeCount() { + return reportedFailureTypes.size(); + } + + private void record(RuntimeException failure) { + swallowedFailures.incrementAndGet(); + String failureType = failure.getClass().getName(); + lastFailureType.set(failureType); + // One line the first time each kind appears. Comparing against the previous kind + // alone would flood at event rate as soon as a broken backend alternates, which a + // timeout that retries as a connection failure does immediately. + if (reportedFailureTypes.size() < REPORTED_FAILURE_TYPE_LIMIT + && reportedFailureTypes.add(failureType)) { + LOGGER.log( + Logger.Level.WARNING, + "GraphRAG telemetry sink {0} is failing with {1}; events are being dropped." + + " Message and stack trace are withheld because a telemetry failure" + + " can quote the event it could not send. Later failures of this kind" + + " are counted rather than logged.", + delegate.getClass().getName(), + failureType); + } + } + + @Override + public String toString() { + return "FailureTolerantGraphRagEventSink{" + delegate + "}"; + } +} diff --git a/components/graph-rag-core/src/main/java/com/orgmemory/graphrag/observability/GraphRagEventSink.java b/components/graph-rag-core/src/main/java/com/orgmemory/graphrag/observability/GraphRagEventSink.java index 885cebf0..cd2f2d63 100644 --- a/components/graph-rag-core/src/main/java/com/orgmemory/graphrag/observability/GraphRagEventSink.java +++ b/components/graph-rag-core/src/main/java/com/orgmemory/graphrag/observability/GraphRagEventSink.java @@ -60,6 +60,16 @@ static GraphRagEventSink composite(List sinks) { }; } + /** + * Wraps a sink so an emission failure is absorbed, counted and reported + * instead of being silently discarded at every call site. + * + * @see FailureTolerantGraphRagEventSink + */ + static FailureTolerantGraphRagEventSink failureTolerant(GraphRagEventSink sink) { + return new FailureTolerantGraphRagEventSink(sink); + } + record GraphRagEvent( UUID operationId, UUID organizationId, diff --git a/components/graph-rag-core/src/test/java/com/orgmemory/graphrag/observability/FailureTolerantGraphRagEventSinkTests.java b/components/graph-rag-core/src/test/java/com/orgmemory/graphrag/observability/FailureTolerantGraphRagEventSinkTests.java new file mode 100644 index 00000000..9e2441ff --- /dev/null +++ b/components/graph-rag-core/src/test/java/com/orgmemory/graphrag/observability/FailureTolerantGraphRagEventSinkTests.java @@ -0,0 +1,106 @@ +package com.orgmemory.graphrag.observability; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; + +import java.time.Duration; +import java.time.Instant; +import java.util.UUID; +import org.junit.jupiter.api.Test; + +/** + * Before this wrapper existed, every producer caught the sink's failure and dropped it, so a + * backend that had been failing since startup was indistinguishable from a quiet one. These + * tests hold both halves of the fix in place: the work still succeeds, and the failure is + * still countable afterwards. + */ +class FailureTolerantGraphRagEventSinkTests { + + @Test + void letsTheObservedWorkSucceedWhenTheBackendIsDown() { + var sink = GraphRagEventSink.failureTolerant(event -> { + throw new IllegalStateException("collector down"); + }); + + assertDoesNotThrow(() -> sink.emit(event())); + } + + @Test + void countsEveryDroppedEventSoABrokenBackendStopsBeingInvisible() { + var sink = GraphRagEventSink.failureTolerant(event -> { + throw new IllegalStateException("collector down"); + }); + + sink.emit(event()); + sink.emit(event()); + sink.emit(event()); + + assertEquals(3, sink.swallowedFailureCount()); + assertEquals(IllegalStateException.class.getName(), sink.lastFailureType()); + } + + @Test + void reportsNothingWhileTheBackendIsHealthy() { + var received = new java.util.ArrayList(); + var sink = GraphRagEventSink.failureTolerant(received::add); + + sink.emit(event()); + + assertEquals(1, received.size(), "a healthy backend must still receive the event"); + assertEquals(0, sink.swallowedFailureCount()); + assertNull(sink.lastFailureType()); + } + + @Test + void keepsCountingAndReportingTheLatestKindWhenTheBackendAlternatesFailures() { + var kinds = new java.util.ArrayDeque(java.util.List.of( + new IllegalStateException("timeout"), + new IllegalArgumentException("connection refused"), + new IllegalStateException("timeout again"))); + var sink = GraphRagEventSink.failureTolerant(event -> { + throw kinds.removeFirst(); + }); + + sink.emit(event()); + sink.emit(event()); + sink.emit(event()); + + // A -> B -> A is what a timeout that retries as a connection failure looks like. + // Every one is counted; the recurrence of A must not produce a second log line, which + // is why reporting is keyed on the set of kinds seen rather than on the previous one. + assertEquals(3, sink.swallowedFailureCount()); + assertEquals(2, sink.reportedFailureTypeCount(), "the recurrence of A must not log again"); + assertEquals(IllegalStateException.class.getName(), sink.lastFailureType()); + } + + @Test + void namesOnlyTheFailureTypeSoATelemetryErrorCannotQuoteTheEventItCouldNotSend() { + var sink = GraphRagEventSink.failureTolerant(event -> { + throw new IllegalStateException( + "POST /v1/traces failed for query 'quarterly revenue for ACME'"); + }); + + sink.emit(event()); + + assertEquals(IllegalStateException.class.getName(), sink.lastFailureType()); + assertFalse(sink.lastFailureType().contains("ACME")); + } + + private static GraphRagEventSink.GraphRagEvent event() { + return new GraphRagEventSink.GraphRagEvent( + UUID.randomUUID(), + UUID.randomUUID(), + GraphRagEventSink.Stage.RETRIEVE, + GraphRagEventSink.Outcome.SUCCEEDED, + Duration.ofMillis(5), + 1, + 1, + null, + null, + null, + null, + Instant.now()); + } +} diff --git a/core/src/main/java/com/orgmemory/core/knowledge/GraphRagKnowledgeRetrievalConfiguration.java b/core/src/main/java/com/orgmemory/core/knowledge/GraphRagKnowledgeRetrievalConfiguration.java index aa970e14..349063ff 100644 --- a/core/src/main/java/com/orgmemory/core/knowledge/GraphRagKnowledgeRetrievalConfiguration.java +++ b/core/src/main/java/com/orgmemory/core/knowledge/GraphRagKnowledgeRetrievalConfiguration.java @@ -40,6 +40,7 @@ GraphRagKnowledgeRetrievalService graphRagKnowledgeRetrievalService( policy, audit, retrievalProperties, - GraphRagEventSink.composite(eventSinks.orderedStream().toList())); + GraphRagEventSink.failureTolerant( + GraphRagEventSink.composite(eventSinks.orderedStream().toList()))); } } diff --git a/docs/increments/active/2026-07-29-observability-pipeline/challenge-brief.md b/docs/increments/active/2026-07-29-observability-pipeline/challenge-brief.md new file mode 100644 index 00000000..38b80d84 --- /dev/null +++ b/docs/increments/active/2026-07-29-observability-pipeline/challenge-brief.md @@ -0,0 +1,104 @@ +# Architecture challenge: payload-free telemetry policy for OrgMemory + +You are an independent architecture reviewer. Your job is to attack the proposal below, +not to validate it. `CLAUDE.md` in this repo requires an independent challenge before +decisions about authorization, persistence, or publication boundaries are implemented — +this is that challenge. Verify claims against the code yourself; do not take the summary +on trust. File paths are given so you can check. + +## What OrgMemory is + +A governed organizational memory layer. Its product promise is permission-aware retrieval: +every chunk of evidence is scoped by an OpenFGA-backed ACL per Knowledge Asset, and +retrieval re-verifies the full evidence closure against the canonical ledger before +anything reaches a model or a user. Multi-tenant, enterprise-facing. + +## The rule under review + +`docs/runbooks/graph-rag-production-hardening.md`, section "Payload-Free Tracing": + +> The application allowlist is limited to operation and organization UUIDs, stage/outcome, +> monotonic duration, bounded input/output counts, an optional lowercase SHA-256 +> model-route fingerprint, and a bounded machine failure code. Never add query, prompt, +> completion, evidence/chunk text, document title/URI, embedding values, actor identity, +> ACL subjects or exception messages. Spring AI prompt and completion observation logging +> is explicitly disabled in API and worker configuration. + +Enforcement is structural, not by convention. See +`components/graph-rag-core/src/main/java/com/orgmemory/graphrag/observability/GraphRagEventSink.java` +— the `GraphRagEvent` record's compact constructor rejects anything outside the allowlist: +fingerprints must match `[0-9a-f]{64}`, `failureCode` must match `[a-z0-9_]{1,64}`, and +there is no field that can carry free text. The OpenTelemetry adapter beside it +(`integrations/graph-rag-observability/.../OpenTelemetryGraphRagEventSink.java`) can only +emit what the record allows. + +## Evidence gathered from comparable systems + +Read from source where marked, otherwise from vendor documentation. + +| System | Prompt/completion leaves the process? | Mechanism | +|---|---|---| +| Dify | Yes, by default | Full input/output history logged; community issue #19345 asks for opt-out docs | +| Onyx (source read at `tmp/onyx`) | Yes, once a provider is configured | `backend/onyx/tracing/framework/span_data.py` — `GenerationSpanData` carries `input`/`output`; `langfuse_tracing_processor.py:178` sends both. `enable_masking=True` by default but `masking.py` only redacts `private_key` / `authorization: bearer` and truncates at 500k — prompt text survives. Provider config per tenant in DB; env-only on multi-tenant cloud | +| Sentry MCP (source read at `tmp/upstream-sentry-mcp-20260726`) | Yes | `sendDefaultPii: true` explicitly set, above Sentry's own `false` default. `packages/mcp-core/src/telem/sentry.ts` adds a `beforeSend` hook with pattern-based `SCRUB_PATTERNS` (API keys, bearer tokens), recursive to depth 20, and logs a warning whenever it actually scrubs something | +| LibreChat | Via proxy | Backend validates the session and strips app auth headers before forwarding telemetry | +| OpenTelemetry GenAI semconv | Opt-in, default off | Single env var `OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT` | +| Spring AI 2.0 | Opt-in, default off | `spring.ai.chat.observations.log-prompt` / `log-completion`, both `matchIfMissing = false` | +| RAGFlow | Yes, via Langfuse integration | Configured in API settings | +| **OrgMemory** | **Never** | Closed allowlist enforced by type | + +No comparable system chose "never". OrgMemory is alone at that end. + +## Observed operational cost + +Production emitted `Failed to publish metrics to OTLP receiver (url=http://localhost:4318/v1/metrics)` +every minute for four days without anyone diagnosing it. Root cause was that +`spring-boot-starter-opentelemetry` transitively pulls `micrometer-registry-otlp`, whose +`OtlpMetricsExportAutoConfiguration` is opt-out and defaults to a localhost URL that +nothing was listening on. Contributing factor: the team's habit is that telemetry does +not say anything useful, partly a product of the policy above. + +## The proposal + +Replace the flat "never" with tiers: + +- **Tier 0** — default, unchanged. Current allowlist, enforced by type. Add a payload-free + counter for how often a redactor had to block something, so silent near-misses become + visible (borrowed from Sentry's scrub-visibility idea). +- **Tier 1** — opt-in per deployment, default off. Exception class and message for + *infrastructure* exceptions, passed through a pattern-based scrubber; hashed actor id. +- **Tier 2** — opt-in per organization, time-boxed, consent recorded. Query and completion + text. Modelled on Onyx: per-tenant config in the database, reloaded without restart. +- **Never at any tier** — evidence/chunk text, document title/URI, ACL subjects, + embedding values. + +A reduced variant is also on the table: **Tier 0 + Tier 1 only**, keeping "never" for +query and completion. This takes the operational win without touching the governance +promise. + +## The strongest counterargument already identified + +An absolute rule is auditable in one line and cannot be misconfigured. Three tiers create +three states to audit, and one environment variable set wrong at Tier 2 leaks customer +content. For a product sold on governance, "cannot be wrong" has real value against +"is not wrong when configured correctly". The current design also enforces the rule in the +type system, which no amount of configuration discipline matches. + +## What to answer + +1. Is the operational-cost argument for Tier 1 actually sound, or is the four-day + undiagnosed warning better explained by something other than the payload policy? + Check whether infrastructure exception detail was genuinely unavailable, or merely + unused — `apps/api/src/main/resources/application-prod.yml` sets log levels, and the + warning did reach stdout. +2. Does the ACL-scoped-evidence distinction actually justify diverging from every + comparable system, or is that reasoning motivated? Attack it. +3. If tiering is adopted, what is the failure mode you would most expect in practice, and + what structural control (not process) prevents it? +4. Is there a fourth option neither variant covers — for example, keeping "never" for the + sink while allowing detail on a separate egress path with different controls? +5. Which variant would you ship: all three tiers, Tier 0+1, or unchanged? Commit to one. + +Answer in prose, be specific about file paths and code you checked, and state plainly +where you think the summary above is wrong or overstated. Disagreement is the useful +output here. diff --git a/docs/increments/active/2026-07-29-observability-pipeline/challenge-verdict.md b/docs/increments/active/2026-07-29-observability-pipeline/challenge-verdict.md new file mode 100644 index 00000000..71894ee3 --- /dev/null +++ b/docs/increments/active/2026-07-29-observability-pipeline/challenge-verdict.md @@ -0,0 +1,780 @@ +# Architecture Verdict: Payload-Free Telemetry Policy for OrgMemory + +Date: 2026-07-29 +Repository: `D:\OrgMemory` +Review basis: commit `86ae6bc02a369471f2b77c87523eb6902988d1ba` on branch `fix/graph-rag-observability-wiring` + +## Committed recommendation + +Ship the **unchanged payload policy**. + +Do not add the proposed Tier 1 exception-message and hashed-actor fields. Do not +add Tier 2 query/completion capture. Keep payload-bearing values structurally +outside `GraphRagEventSink`. + +“Unchanged” here means unchanged policy, not an assertion that the present +application is already proven payload-free. The custom GraphRAG event adapter is +narrow, but Spring AI observations, Micrometer error spans, provider library +WARN paths, global OpenTelemetry resource data, and ordinary application logs +sit outside that type boundary. Those paths must be audited and hardened before +the deployment is described as globally payload-free. + +For richer operational troubleshooting, use the fourth option described below: +a separate diagnostic path with its own data type, endpoint, credentials, +retention, access controls, and failure semantics. + +## Verification performed + +The verdict is based on direct inspection of the current repository, the +resolved dependency sources used by the build, the checked-in upstream +comparables under `tmp`, and focused tests. + +The focused tests passed: + +```text +.\gradlew.bat --no-daemon \ + :components:graph-rag-core:test \ + --tests "*GraphRagEventSinkTests" \ + :integrations:graph-rag-observability:test \ + --tests "*OpenTelemetryGraphRagEventSinkTests" +``` + +Gradle dependency inspection resolved: + +```text +org.springframework.boot:spring-boot-starter-opentelemetry:4.1.0 +io.micrometer:micrometer-registry-otlp:1.17.0 +io.micrometer:micrometer-tracing-bridge-otel:1.7.0 +org.springframework.ai:spring-ai-model:2.0.0 +org.springframework.ai:spring-ai-openai:2.0.0 +org.springframework.ai:spring-ai-anthropic:2.0.0 +``` + +Context7 was attempted for current framework documentation but its monthly +quota was exhausted. Framework behavior was therefore checked against the +actual resolved source JARs plus official Spring AI, Spring Boot, Micrometer, +OpenTelemetry, and Sentry documentation. + +## Claim-by-claim audit + +### 1. Permission-aware evidence closure + +**Challenge claim:** every contributing evidence chunk is authorization-scoped +and the complete evidence closure is re-verified against the canonical ledger +before anything reaches a model. + +**Verdict:** substantially verified for the GraphRAG retrieval path. + +Evidence: + +- `core/src/main/java/com/orgmemory/core/knowledge/GraphRagKnowledgeRetrievalService.java:271-331` + re-resolves the current scope, constructs the complete grounding closure, + checks its size, verifies it through OpenFGA, canonical-rechecks it, compares + the rechecked evidence identity, and only then calls the grounding renderer. +- `core/src/main/java/com/orgmemory/core/knowledge/GraphRagKnowledgeRetrievalService.java:666-726` + performs the exact OpenFGA `BatchCheck` and canonical recheck. +- `core/src/main/java/com/orgmemory/core/knowledge/GraphRagKnowledgeRetrievalService.java:810-836` + compares organization, asset, source revision, ACL snapshot, and projection + generation identities. +- `core/src/main/java/com/orgmemory/core/knowledge/SecureKnowledgeRetrievalStore.java:92-212` + applies organization, lifecycle, publication/model, sealed-ingestion ACL, + current ACL, deny/allow, mapped source user/group, and classification filters. +- `core/src/test/java/com/orgmemory/core/knowledge/GraphRagKnowledgeRetrievalServiceTests.java:214-492` + covers revocation, entity/relation/direct-chunk contributions, canonical + recheck, model mismatch, and prevention of model egress on failure. +- `docs/guidelines/agent-safety.md:11-14` states that generated summaries, + facts, and other derivatives inherit the permissions of every contributing + evidence item. + +The claim should still be scoped to this implemented retrieval path rather than +presented as a theorem about every possible future model invocation. + +### 2. The runbook accurately describes the `GraphRagEvent` schema + +**Challenge claim:** the application allowlist consists of operation and +organization UUIDs, stage/outcome, monotonic duration, bounded input/output +counts, an optional model-route fingerprint, and a bounded failure code. + +**Verdict:** incomplete and partially wrong. + +`components/graph-rag-core/src/main/java/com/orgmemory/graphrag/observability/GraphRagEventSink.java:63-75` +actually defines: + +```text +operationId +organizationId +stage +outcome +duration +inputCount +outputCount +modelRouteFingerprint +scopeFingerprint +cacheStatus +failureCode +occurredAt +``` + +The runbook at +`docs/runbooks/graph-rag-production-hardening.md:23-29` omits +`scopeFingerprint`, `cacheStatus`, and the explicit occurrence time. + +Specific corrections: + +- Counts are only checked for non-negativity at + `GraphRagEventSink.java:86-88`. There is no policy maximum, bucketing, or + stage-specific bound. Java `int` is a representation bound, not the + “bounded counts” policy described by the runbook. +- The record only rejects a negative `Duration` at + `GraphRagEventSink.java:82-85`. Current producers generally calculate + duration from `System.nanoTime()`, but the type itself cannot prove monotonic + provenance. +- `failureCode` is matched against `[a-z0-9_]{1,64}` at + `GraphRagEventSink.java:17-18,102-110`. This limits syntax and length, but it + is not a closed error taxonomy. Any conforming arbitrary token can be passed. +- Failed outcomes require a code, but successful or cancelled outcomes are not + forbidden from carrying one. +- Fingerprints are required to look like lowercase SHA-256 at + `GraphRagEventSink.java:89-100`, but the type cannot prove what was hashed. A + future producer could hash query text and create a stable equality oracle + while still satisfying the constructor. + +Current producer provenance appears intentional: + +- `apps/api/src/main/java/com/orgmemory/api/graphrag/GraphRagRuntimeConfiguration.java:78-82` + hashes the configured gateway/model route. +- `core/src/main/java/com/orgmemory/core/knowledge/GraphRagKnowledgeRetrievalService.java:621-629` + hashes organization and knowledge-space scope. +- `core/src/main/java/com/orgmemory/core/knowledge/GraphRagKnowledgeRetrievalService.java:769-771` + hashes the rerank provider. + +That is a useful current convention, but it is not enforced by the event type. + +### 3. The custom record and adapter structurally exclude direct payload text + +**Challenge claim:** the compact constructor rejects anything outside the +allowlist, there is no field for free text, and the OpenTelemetry adapter can +only emit what the record permits. + +**Verdict:** verified for direct fields on this one custom adapter, overstated as +an application-wide guarantee. + +The custom path is narrow: + +- `components/graph-rag-core/src/main/java/com/orgmemory/graphrag/observability/GraphRagEventSink.java:63-112` + has no query, prompt, completion, evidence, title, URI, actor, ACL subject, or + exception-message field. +- `integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/OpenTelemetryGraphRagEventSink.java:23-45` + declares eleven explicit attribute keys. +- `OpenTelemetryGraphRagEventSink.java:54-91` emits only those event-derived + attributes, span name, status, and timing. Failed custom events set ERROR + status without calling `recordException`. +- `integrations/graph-rag-observability/src/test/java/com/orgmemory/integrations/graphrag/observability/OpenTelemetryGraphRagEventSinkTests.java:22-80` + verifies the exact eleven-key attribute set for one manually constructed + custom span. + +The absolute claim is nevertheless too strong: + +- `failureCode` remains an open string constrained only by character class and + length. +- The OpenTelemetry SDK adds span identity, parent/trace context, resource + attributes, instrumentation scope, timestamps, and status outside the record. +- Span processors and other instrumentation share the exporter and can add or + export unrelated data. +- The custom test does not inspect resource attributes, span events, parent + spans, Spring AI spans, HTTP spans, real application wiring, exporter + behavior, or logs. +- The adapter starts a span with the current context rather than explicitly + creating a root span, so it can be exported as part of a larger trace whose + other spans have a different data policy. + +The runbook itself recognizes that a whole-export test is still required: +`docs/runbooks/graph-rag-production-hardening.md:31-38` requires scanning every +attribute and event and confirming that no exception event or stack trace is +present. The narrow unit test does not satisfy that release gate. + +### 4. Spring AI prompt and completion observation logging is disabled + +**Challenge claim:** Spring AI prompt and completion observation logging is +explicitly disabled in API and worker configuration. + +**Verdict:** verified as the repository default, but not structural and not +sufficient to prove application-wide payload-free egress. + +Evidence: + +- `apps/api/src/main/resources/application.yml:4-13` sets: + + ```yaml + spring: + ai: + chat: + observations: + log-prompt: false + log-completion: false + ``` + +- `apps/worker/src/main/resources/application.yml:20-24` sets the same values. +- Spring AI 2.0's resolved auto-configuration creates the prompt/completion + logging handlers only when the relevant property equals `true`. +- The official Spring AI observability documentation also describes prompt and + completion content as sensitive and disabled by default: + . + +However: + +- External configuration can override the YAML values. +- The settings control Spring AI's content logging handlers, not every + observation field, error event, provider-library WARN, or application log. +- The application programmatically supplies the real shared + `ObservationRegistry` to both model implementations: + - `integrations/ai-model-gateways/src/main/java/com/orgmemory/integrations/ai/gateway/openai/OpenAiCompatibleChatModelFactory.java:20-45` + - `integrations/ai-model-gateways/src/main/java/com/orgmemory/integrations/ai/gateway/anthropic/AnthropicMessagesChatModelFactory.java:21-46` + +Resolved Spring AI 2.0 source +`org/springframework/ai/chat/observation/DefaultChatModelObservationConvention.java:40-116` +therefore emits `gen_ai.client.operation` observations containing, among other +things: + +- request and response model IDs; +- response IDs; +- stop sequences and tool names; +- temperature, token limits, penalties, and streaming status; +- finish reasons and token counts. + +Those fields do not normally contain the full prompt, but raw model identifiers +already bypass the runbook's model-route fingerprint policy. + +More seriously, resolved Micrometer tracing source +`io/micrometer/tracing/handler/TracingObservationHandler.java:133-137` calls +`Span.error(Throwable)` for observation errors. Resolved +`micrometer-tracing-bridge-otel:1.7.0` source +`io/micrometer/tracing/otel/bridge/OtelSpan.java:170-176` records the complete +exception event and sets the OpenTelemetry error-status description to +`throwable.getMessage()`. + +That contradicts an application-wide reading of the runbook's requirement that +error spans contain no exception event or stack trace. + +### 5. OrgMemory currently has no payload escape paths outside the custom sink + +**Challenge implication:** the closed `GraphRagEvent` type makes the whole +application's observability payload-free. + +**Verdict:** wrong. + +In the resolved Spring AI 2.0 provider source: + +- `org/springframework/ai/openai/OpenAiChatModel.java:219-224` logs + `"No choices returned for prompt: " + prompt` at WARN. +- `org/springframework/ai/anthropic/AnthropicChatModel.java:551-556` logs + `"No content blocks returned for prompt: " + prompt` at WARN. +- `AnthropicChatModel.java:1015-1019` logs unsupported content blocks. +- `AnthropicChatModel.java:1216-1221` logs the raw malformed tool-argument JSON + plus the exception. + +The application's prompts can contain exactly the categories forbidden by the +runbook: + +- `integrations/graph-rag-spring-ai/src/main/java/com/orgmemory/integrations/graphrag/springai/SpringAiQueryAnswerModel.java:57-69` + sends the raw query and verified grounded prompt. +- `integrations/graph-rag-spring-ai/src/main/java/com/orgmemory/integrations/graphrag/springai/SpringAiKeywordPlanningModel.java:35-44` + sends a prompt containing the query. +- `integrations/graph-rag-spring-ai/src/main/java/com/orgmemory/integrations/graphrag/springai/SpringAiExtractionModel.java:47-61` + sends chunk and extraction-conversation content. +- `integrations/graph-rag-spring-ai/src/main/java/com/orgmemory/integrations/graphrag/springai/SpringAiDescriptionSummaryModel.java:34-56` + sends evidence-derived descriptions. + +Production config permits those WARNs: + +- `apps/api/src/main/resources/application-prod.yml:81-86` sets root logging to + INFO by default. +- `apps/worker/src/main/resources/application-prod.yml:46-50` does the same. + +Therefore `log-prompt:false` does not suppress these provider-library WARN +paths. The custom event type cannot protect a log statement that never touches +that type. + +There is already an intentional separate diagnostic path in worker indexing: + +- `apps/worker/src/main/java/com/orgmemory/worker/graph/GraphIndexingProcessor.java:202-265` + emits a bounded machine failure code to `GraphRagEventSink`. +- `GraphIndexingProcessor.java:268-282` logs diagnostic detail separately. + +That separation is architecturally useful, but the logging channel still needs +its own access, retention, and payload audit. + +### 6. Exact counts and stable fingerprints are harmless + +**Challenge implication:** counts and SHA-256 fingerprints are unconditionally +safe payload-free fields. + +**Verdict:** overstated. + +Exact counts can disclose organization or corpus topology, including authorized +asset, evidence-closure, chunk, entity, relation, or keyword cardinalities. +Stable unkeyed hashes allow cross-event correlation and dictionary testing when +the input domain is low entropy. + +The current values are materially safer than text, but production policy should +consider: + +- stage-specific upper bounds or buckets; +- domain-separated keyed hashes rather than bare SHA-256; +- explicit constructors for route and scope fingerprints so arbitrary producers + cannot hash payload text; +- retention and access limits even for “payload-free” telemetry. + +### 7. The comparable-systems table establishes an industry baseline + +**Challenge claim:** every comparable system permits prompt/completion egress, +so OrgMemory is alone at “never.” + +**Verdict:** not established. The table mixes unlike mechanisms and several +rows are overstated. + +#### Dify + +The cited issue reports that a +user observed several telemetry domains and asks for documentation and opt-out +instructions. It does not demonstrate that full prompt and completion history +is exported by default. Configured Langfuse workflow tracing is different from +unconditional native telemetry. + +The challenge's “Yes, by default” conclusion is unsupported by that citation. + +#### Onyx + +The main capture claim is verified: + +- `tmp/onyx/backend/onyx/tracing/framework/span_data.py:89-146` defines + generation span data containing input, output, and reasoning. +- `tmp/onyx/backend/onyx/tracing/langfuse_tracing_processor.py:295-321` + sends input/output and other generation information. +- `tmp/onyx/backend/onyx/tracing/langfuse_tracing_processor.py:143-149` + sends user and session IDs. +- `tmp/onyx/backend/onyx/tracing/masking.py:7-71` only handles a narrow set of + private-key/authorization patterns and a 500,000-character limit. Normal + prompt text survives. +- `tmp/onyx/backend/onyx/tracing/langfuse_tracing_processor.py:76-86` fails open: + if masking throws, it logs a warning and returns the original unmasked value. +- Error status data at + `tmp/onyx/backend/onyx/tracing/langfuse_tracing_processor.py:335-340` is not + passed through the same masking helper. + +Live reload is also real: + +- `tmp/onyx/backend/onyx/tracing/dynamic_processor.py:104-159` reloads provider + configuration. +- `tmp/onyx/backend/onyx/configs/app_configs.py:1618-1619` gives the cache a + short TTL. + +But the proposed “per-organization database setting modelled on Onyx” is wrong: + +- `tmp/onyx/backend/onyx/tracing/provider_config.py:123-128` explicitly bypasses + database configuration in `MULTI_TENANT` mode and uses environment-wide + settings. +- `tmp/onyx/backend/onyx/server/manage/tracing/api.py:36-46` rejects tracing + configuration management in multi-tenant mode. +- In non-multi-tenant mode, the database model holds one unique configuration + row per provider rather than a SaaS organization-by-organization consent + model. + +Onyx is therefore evidence that live-reloaded capture and narrow masking exist. +It is not evidence that a per-organization Tier 2 is safe or already solved. + +Onyx is also permission-aware: + +- `tmp/onyx/backend/onyx/context/search/preprocessing/access_filters.py:8-22` + applies user access filters to search. +- `tmp/onyx/backend/onyx/db/document_access.py:26-83` filters by public, + email, and group ACL. + +OrgMemory's full canonical closure recheck is stronger, but “permission-aware +retrieval is unique to OrgMemory” is not true. + +#### Sentry MCP + +The repository confirms conditional prompt/output capture, but the challenge +overstates it as unconditional and overstates the protection provided by its +scrubber. + +- `tmp/upstream-sentry-mcp-20260726/TELEMETRY.md:258-265` says Node/stdio and + Cloudflare telemetry are disabled when the DSN is absent. +- `tmp/upstream-sentry-mcp-20260726/packages/mcp-server/src/index.ts:279-302` + explicitly enables Vercel AI input/output recording when Sentry is + configured. +- `tmp/upstream-sentry-mcp-20260726/packages/mcp-core/src/telem/sentry.ts:7-28` + has only three secret patterns and a recursion depth of twenty. +- `tmp/upstream-sentry-mcp-20260726/packages/mcp-core/src/telem/sentry.ts:34-117` + recursively scrubs events and warns when it changes an event. + +The custom hook is `beforeSend`; the source contains no `beforeSendSpan`. +Sentry's official documentation exposes `beforeSendSpan` as the span-specific +hook. Therefore the event scrubber cannot be treated as structural protection +for GenAI span input/output: +. + +This is evidence that scrub visibility is useful, but also that a narrow +pattern scrubber is not a proof of absence. + +#### LibreChat + +The described backend RUM proxy validates the session and removes application +authorization headers before forwarding browser telemetry. That is a browser +RUM transport boundary, not proof that prompt/completion content is routinely +captured or a comparable design for ACL-derived model output. + +#### OpenTelemetry GenAI semantic conventions + +Content capture is commonly opt-in and disabled by instrumentation defaults, +but describing it as one universal semantic-convention environment variable is +too broad. Semantic conventions define names and meanings; individual +instrumentations decide which switches and defaults they implement. Official +OpenTelemetry material documents opt-in message-content capture, but it is not +a universal structural policy for every SDK and instrumentation: +. + +#### Spring AI + +The Spring AI row is substantially correct: prompt and completion content +logging is opt-in/default-off. However, it does not imply that model metadata, +exception events, status descriptions, or provider-library WARNs are absent. + +#### RAGFlow + +The inspected material supports the existence of a configured Langfuse +integration. It does not establish unconditional default prompt/completion +egress, nor does it establish governance equivalence with OrgMemory. + +#### OrgMemory + +The table heading must be qualified as “leaves the process through +observability.” Literally, prompts necessarily leave the application for the +configured model provider: + +- `core/src/main/java/com/orgmemory/core/assistant/AssistantService.java:58-63` + sends the verified request through `ChatModelPort`. + +Even under the intended observability-only interpretation, “Never” is currently +false as an application-wide statement because the Spring AI observation, +Micrometer exception, and WARN paths described above bypass the custom sink. + +### 8. The four-day OTLP warning demonstrates the operational cost of the payload policy + +**Challenge claim:** the warning remained undiagnosed partly because telemetry +does not say anything useful, supporting Tier 1. + +**Verdict:** the dependency/default mechanism is verified; the causal conclusion +is unsupported. + +Verified mechanism: + +- `apps/api/build.gradle.kts:23` and `apps/worker/build.gradle.kts:15` add + `spring-boot-starter-opentelemetry`. +- Gradle resolves + `micrometer-registry-otlp:1.17.0 <- spring-boot-starter-opentelemetry:4.1.0`. +- Resolved Spring Boot source + `org/springframework/boot/micrometer/metrics/autoconfigure/export/otlp/OtlpMetricsExportAutoConfiguration.java:58-65` + uses `@ConditionalOnEnabledMetricsExport("otlp")`. +- Resolved Spring Boot source + `OnMetricsExportEnabledCondition.java:39-77` defaults exporters to enabled + when no product-specific or default disable property exists. +- Resolved Micrometer source + `io/micrometer/registry/otlp/OtlpConfig.java:54-77` defaults to + `http://localhost:4318/v1/metrics`. +- Resolved Micrometer source + `io/micrometer/registry/otlp/OtlpMeterRegistry.java:193-213` catches the + publishing exception and calls `logger.warn(message, exception)`. The + configuration context includes the destination URL and resource attributes. + +Repository deployment configuration does not supply an OTLP metrics endpoint or +explicitly disable the exporter: + +- `apps/api/src/main/resources/application-prod.yml:70-79` +- `apps/worker/src/main/resources/application-prod.yml:37-44` +- `infrastructure/deployment/compose.production.yaml:21-53` +- API and worker inherit the shared environment at + `infrastructure/deployment/compose.production.yaml:329-333,376-380`. + +Exception detail was therefore available in the logging call, and production +root INFO admits WARN. Docker's `json-file` logging configuration captures +stdout/stderr at `infrastructure/deployment/compose.production.yaml:3-14`. + +Even the one-line warning identified: + +- the failed subsystem: OTLP metrics publishing; +- the exact destination: localhost port 4318; +- the metrics endpoint path. + +The repository provides a stronger explanation for delayed detection: + +- `infrastructure/deployment/scripts/smoke-production.sh:31-147` checks health + and functional HTTP behavior, but not exporter delivery or repeated log + failures. +- `infrastructure/deployment/scripts/deploy.sh:213-226` declares deployment + success after those smokes. +- The checked-in infrastructure contains no corresponding collector-delivery or + log-alert verification. + +The repository history makes four days temporally plausible because the starter +was introduced on 2026-07-25, but it does not prove continuous duration, who +observed the warning, or that a team belief that telemetry is useless caused the +delay. + +Most importantly, the OTLP meter registry never passes through +`GraphRagEventSink`. Adding exception messages to `GraphRagEvent` would neither +enrich nor detect that metrics warning. Sending the diagnostic through the same +broken OTLP route would also be circular. + +## Answers to the five questions + +## 1. Is the operational-cost argument for Tier 1 sound? + +No. + +The incident demonstrates three operational problems: + +1. a transitive exporter was enabled without an intentional endpoint; +2. deployment validation did not verify telemetry delivery or scan for exporter + failures; +3. repeated WARNs lacked alert ownership. + +It does not demonstrate that infrastructure exception detail was unavailable. +`OtlpMeterRegistry` logged the full caught exception, production log levels +admitted it, and the warning's own message contained the failing URL. + +It also does not demonstrate a limitation of `GraphRagEventSink`, because the +warning originates in an independent Micrometer registry publisher. Tier 1 +would not have changed that code path. + +The appropriate fixes are: + +- set `management.otlp.metrics.export.enabled=false` when no receiver exists; +- when enabled, require an explicit endpoint and fail deployment or startup if + the endpoint is absent; +- add a smoke check that exports a known metric/span and verifies receipt; +- alert on repeated exporter WARNs and dropped-signal indicators; +- define responsibility for collector and exporter health. + +There is a separate real issue: GraphRAG event producers often catch and ignore +sink failures: + +- `core/src/main/java/com/orgmemory/core/knowledge/GraphRagKnowledgeRetrievalService.java:149-172,565-610,630-646,772-788` +- `apps/worker/src/main/java/com/orgmemory/worker/graph/GraphIndexingProcessor.java:249-265` + +That should produce a local payload-free health counter or bounded log signal. +It still does not justify putting arbitrary exception messages into the remote +GraphRAG telemetry type. + +## 2. Does ACL-scoped evidence justify diverging from every comparable system? + +The argument is partly motivated and partly sound. + +The motivated part is the assertion of uniqueness: + +- permission-aware retrieval is not unique to OrgMemory; +- the comparable table does not establish that every peer exports payload by + default; +- the rows describe different mechanisms and threat models; +- competitor behavior would not by itself establish a safe authorization or + persistence design. + +The sound part is the local authorization consequence: + +- a query can contain newly supplied confidential material that is not yet a + governed Knowledge Asset; +- a completion can quote, combine, or paraphrase material from several assets; +- the completion inherits the effective permissions of every contributor; +- an organization-level consent flag is coarser than per-asset and current + source ACLs; +- consent expiry does not retract data already exported to a telemetry backend; +- an ordinary trace viewer cannot enforce current source revocation, deletion, + retention, evidence-closure authorization, or operator access. + +Tier 2 would therefore create a second persistence/publication plane whose +authorization semantics do not match the plane that allowed the model request. + +The defensible principle is not “OrgMemory is unique, therefore content must +never exist anywhere.” It is: + +> Query and completion content must not enter an ordinary observability store +> unless that store models the same authorization, retention, revocation, +> deletion, and operator-access semantics as the governed evidence and its +> derivatives. + +Under the proposed design, it does not. That is enough to reject Tier 2. + +## 3. If tiering is adopted, what failure is most likely, and what structural control prevents it? + +The most likely failure is **tier bleed caused by stale or missing tenant +context**. + +Examples: + +- a shared/global observation handler sees a Tier 2 flag and captures another + organization's request; +- a cached organization setting remains enabled after consent expires; +- asynchronous work loses the organization identity but retains capture state; +- a database setting changes while a trace is in flight; +- a global environment override silently wins over the database; +- a payload-bearing event enters the ordinary Tier 0 exporter; +- the telemetry backend retains payload after the capture lease ends; +- a scrubber misses content in a nested exception, provider body, URL, filename, + SQL message, tool argument, alternate encoding, or unknown secret format; +- the scrubber itself fails and returns the original data. + +This risk is especially high in the current application because: + +- OpenAI and Anthropic model factories share the application + `ObservationRegistry`; +- GraphRAG model construction and some model invocations do not consistently + carry an organization-bound capture capability; +- the proposed Onyx precedent is global in multi-tenant mode, not per + organization. + +If tiering were nevertheless adopted, the structural control must be a +physically and logically distinct payload capability: + +1. `GraphRagEvent` remains incapable of carrying content. +2. Payload capture uses a different sealed type and API. +3. Capture requires a non-forgeable, organization- and operation-bound + capability containing consent version and expiry. +4. The capability is revalidated immediately before capture and again before + egress; absent or stale context fails closed. +5. No environment variable or process-global Boolean may enable tenant payload + capture. +6. The payload path uses a separate process or exporter, endpoint, credentials, + network policy, queue, encryption key, storage, and RBAC. +7. The destination enforces hard TTL, deletion, tenant partitioning, access + audit, and lease expiry. +8. Completion capture additionally carries the evidence-closure identity and + enforces current authorization for every viewer. + +For Tier 1 specifically, arbitrary exception messages should not be admitted at +all. Use a closed or sealed infrastructure taxonomy with safe structured fields +such as dependency alias, status family, timeout, retry count, errno, and stack +fingerprint. Pattern scrubbing is defense in depth, not a structural +authorization or data-absence control. + +The proposed Tier 0 redactor counter also needs correction. The existing sink +does not receive payload and has no redactor. To count rejected near-misses, the +system would first need to create a raw candidate data path, weakening the +current type boundary. Prefer: + +- closed construction APIs; +- semantic tests for every permitted field; +- whole-export allowlist/denylist tests; +- collector-side defense-in-depth filtering; +- local counters for invalid event construction or rejected attributes, without + carrying the rejected value. + +## 4. Is there a fourth option? + +Yes. Keep the GraphRAG OTLP sink payload-free and create a separate, +deployment-controlled diagnostics path. + +The ordinary remote telemetry path should retain: + +- operation and organization identifiers under the approved identity policy; +- stage and outcome; +- bounded or bucketed counts; +- duration; +- closed failure codes; +- safe route/scope correlation values; +- exporter health counters. + +The diagnostic path should have a different schema and governance boundary. Its +default structured envelope can contain: + +- sealed exception class or error family; +- subsystem and dependency alias; +- status-code family or errno; +- timeout, retryability, and attempt count; +- deployment or build identity; +- bounded cause-chain taxonomy; +- stack fingerprint, not stack text. + +It should use: + +- different endpoint and credentials; +- separate network egress policy; +- short retention; +- restricted operator access; +- explicit access audit; +- encryption; +- independent availability and failure alerts. + +Full stack details can remain in protected local/deployment logs where justified, +provided those logs receive their own payload audit and retention controls. +Existing worker code already separates the machine-code event from diagnostic +logging in `GraphIndexingProcessor.java:221-282`. + +If a rare support case truly requires query, completion, or evidence-derived +content, it should not be “Tier 2 telemetry.” It should be an explicit, +single-operation support capture represented as a governed derived resource: + +- organization- and operation-bound; +- authorized under OpenFGA or an equivalent current-policy check; +- associated with the evidence closure; +- encrypted and stored in a dedicated support store; +- accessible only to approved support principals; +- fully audited; +- subject to hard TTL and verified deletion. + +That is a new governed product capability and should receive its own +authorization, persistence, and publication challenge. It is not a telemetry +configuration flag. + +## 5. Which variant should ship? + +Ship **unchanged** among the offered variants. + +Reject all three tiers because Tier 2's organization-level consent cannot model +asset-level ACL, evidence inheritance, revocation, deletion, retention, or +operator access. + +Reject Tier 0 + Tier 1 because arbitrary exception messages remain payload. A +pattern scrubber cannot prove that unknown customer text, credentials, provider +bodies, filenames, SQL, URLs, tool arguments, nested causes, or alternative +encodings were removed. The operational incident cited in the proposal also +would not have been improved by Tier 1. + +Retain the current payload-free GraphRAG event policy, but do not ship a release +claiming global enforcement until the following implementation gaps are closed: + +1. Explicitly disable OTLP metrics when no receiver is configured and require a + valid endpoint when enabled. +2. Audit or suppress Spring AI's standard model observations when they violate + the GraphRAG allowlist, including raw model IDs and exception events. +3. Prevent provider-library WARN paths from logging raw prompts, response + blocks, or tool arguments. +4. Replace open-string `failureCode` with a closed taxonomy. +5. Make route/scope fingerprint construction domain-specific and preferably + keyed/domain-separated. +6. Bound or bucket counts according to stage and disclosure risk. +7. Add a payload-free local health signal when custom event emission fails. +8. Add a real application/exporter integration test that scans: + - every span and event; + - resource and instrumentation-scope attributes; + - Spring AI and HTTP spans; + - success and failure paths; + - relevant stdout/stderr logs; + - actual collector output. +9. Retain sanitized export evidence before claiming the runbook's payload-free + production gate has passed. + +## Final decision statement + +The operational incident does not justify weakening the GraphRAG telemetry +boundary. The custom event type is valuable precisely because payload cannot be +added through ordinary configuration, but the repository currently overstates +how far that protection extends. + +The selected architecture is: + +> Keep `GraphRagEventSink` payload-free and reject Tier 1 and Tier 2. Close the +> existing Spring AI, Micrometer, and logging bypasses. Put richer operational +> diagnostics on a separate, access-controlled egress path. Treat any future +> content capture as a governed, short-lived derived resource—not telemetry. + diff --git a/docs/increments/active/2026-07-29-observability-pipeline/design.md b/docs/increments/active/2026-07-29-observability-pipeline/design.md new file mode 100644 index 00000000..d4249439 --- /dev/null +++ b/docs/increments/active/2026-07-29-observability-pipeline/design.md @@ -0,0 +1,237 @@ +# Observability pipeline and payload boundary + +Date: 2026-07-29 + +## Outcome + +Give production a telemetry pipeline that reaches a real collector, stop the +payload paths that bypass the GraphRAG event boundary, and stop the exporter +failure that has been logging every minute since 2026-07-25. + +## Production evidence + +`orgmemory-api-1` and `orgmemory-worker-1` log every minute: + +```text +i.m.registry.otlp.OtlpMeterRegistry : Failed to publish metrics to OTLP receiver +(context: url=http://localhost:4318/v1/metrics, resource-attributes={service.name=orgmemory-api}) +``` + +The mechanism is a transitive default, not a misconfiguration: + +- `apps/api/build.gradle.kts:23` and `apps/worker/build.gradle.kts:15` add + `spring-boot-starter-opentelemetry`, which resolves + `io.micrometer:micrometer-registry-otlp:1.17.0`. +- `OtlpMetricsExportAutoConfiguration` is `@ConditionalOnEnabledMetricsExport("otlp")`, + and `OnMetricsExportEnabledCondition` treats exporters as enabled unless a + property disables them. Metrics export is opt-out. +- Micrometer's `OtlpConfig.url()` defaults to `http://localhost:4318/v1/metrics`. +- No repository configuration sets an OTLP metrics endpoint. + +OTLP **tracing** behaves oppositely and is not involved: `OtlpTracingConfigurations` +gates its connection-details bean on +`management.opentelemetry.tracing.export.otlp.endpoint`, so no endpoint means no +exporter and no log noise. + +`localhost:4318` inside the API container is that container, so the export never +had a destination. The ZM host does run a Grafana stack — +`zeromail-alloy`, `zeromail-prometheus`, `zeromail-loki`, `zeromail-tempo`, +`zeromail-grafana`, all `Exited` since 2026-07-25 — but `zeromail-alloy` publishes +4317/4318 to the host loopback on network `zeromail-internal`, which OrgMemory +containers do not join. Its shutdown is coincident, not causal. + +Readiness stayed `UP` throughout, which is correct: exporter delivery is not a +readiness signal. + +## Payload boundary: challenge and finding + +`docs/runbooks/graph-rag-production-hardening.md` states that telemetry never +carries query, prompt, completion, evidence text, document title/URI, embedding +values, actor identity, ACL subjects, or exception messages. A proposal to +replace that flat rule with three configurable tiers was put through an +independent architecture challenge (Codex, `gpt-5.6-sol`, ultra effort). The +challenge brief and full verdict were produced during the review; their +substance is recorded here because that scratch location is not versioned. + +### Proposal + +- Tier 0: current allowlist, unchanged, plus a counter for redactor near-misses. +- Tier 1: opt-in per deployment — infrastructure exception class and message + through a pattern scrubber, hashed actor id. +- Tier 2: opt-in per organization, time-boxed, consent recorded — query and + completion text, modelled on Onyx. +- Never at any tier: evidence/chunk text, document title/URI, ACL subjects, + embedding values. + +Supporting argument: no comparable open-source system chose "never". Dify, Onyx, +Sentry MCP, LibreChat and RAGFlow all permit content egress; OpenTelemetry's +GenAI conventions and Spring AI make it opt-in and default-off. The operational +cost of the strict rule was argued from the four-day undiagnosed OTLP warning. + +### Strongest counterargument + +An absolute rule is auditable in one line and cannot be misconfigured. Three +tiers create three states to audit, and one environment variable set wrong at +Tier 2 leaks customer content. The current design enforces the rule in the type +system, which configuration discipline cannot match. + +### Repository evidence produced by the challenge + +The review rejected the proposal and produced two verified findings that +outweigh it. Both were independently confirmed against the resolved sources. + +**Provider libraries log raw prompts.** `log-prompt: false` disables Spring AI's +observation handlers, not the provider libraries' own logging: + +```text +org/springframework/ai/openai/OpenAiChatModel.java:222 + logger.warn("No choices returned for prompt: " + prompt); +org/springframework/ai/anthropic/AnthropicChatModel.java:554 + logger.warn("No content blocks returned for prompt: " + prompt); +org/springframework/ai/anthropic/AnthropicChatModel.java:1219 + logger.warn("Failed to parse tool arguments JSON: " + argumentsJson, e); +``` + +OrgMemory prompts carry exactly the forbidden categories — +`SpringAiQueryAnswerModel` sends the query and grounded prompt, +`SpringAiExtractionModel` sends chunk content, +`SpringAiDescriptionSummaryModel` sends evidence-derived descriptions. Production +root logging is INFO, so WARN is emitted, and Compose retains stdout in +`json-file` logs on the host. + +**Error spans carry exception events and messages.** +`io/micrometer/tracing/otel/bridge/OtelSpan.java:170-179` calls +`recordException(throwable)` and sets the status description to +`throwable.getMessage()`. The runbook's release gate — "confirm error spans +contain no exception event or stack trace" — cannot pass with the current wiring. + +The review also corrected the proposal's own evidence: the runbook omits three +fields the record actually carries (`scopeFingerprint`, `cacheStatus`, +`occurredAt`); "bounded counts" is only a non-negativity check; and Onyx's +tracing configuration is environment-wide in `MULTI_TENANT` mode +(`provider_config.py:123-128`), so it is not a precedent for per-organization +consent. + +## Decision + +**Keep the payload-free policy. Reject Tier 1 and Tier 2.** + +The governing principle is not that OrgMemory is unique — permission-aware +retrieval is not unique, and Onyx applies access filters too. It is: + +> Query and completion content must not enter an ordinary observability store +> unless that store models the same authorization, retention, revocation, +> deletion, and operator-access semantics as the governed evidence and its +> derivatives. + +A completion inherits the effective permissions of every contributing evidence +item. An organization-level consent flag is coarser than per-asset ACLs, consent +expiry cannot retract exported data, and a trace viewer cannot enforce current +revocation. That is sufficient to reject Tier 2. Tier 1 is rejected because an +arbitrary exception message is payload, and a pattern scrubber cannot prove that +customer text, provider bodies, filenames, SQL, URLs, or tool arguments were +removed. + +The Tier 0 redactor counter is also rejected: the sink has no redactor because +payload cannot reach it. Counting near-misses would require building a raw +candidate path, weakening the boundary it was meant to observe. + +**Rejected alternative:** enabling `spring.ai.chat.observations.log-prompt` and +exporting to Langfuse. Langfuse remains supported as an OTLP destination for +payload-free spans; it needs no adapter code, only an endpoint and headers. + +**Accepted alternative for operability:** richer diagnostics belong on a separate +egress path with its own schema (sealed exception family, dependency alias, +status family, retry count, stack fingerprint rather than stack text), +credentials, retention, and access control — not inside `GraphRagEventSink`. + +The policy is unchanged but the repository currently claims more than it +enforces. The bypasses above must close before any release describes the +deployment as payload-free. + +Nothing has leaked yet. The project owner confirmed on 2026-07-29 that the ZM +deployment is a proof of concept carrying no real users and no customer data, so +the retained logs hold no exposure to scope and closing the bypasses is +preventive. That is a statement about today's deployment, not about the design: +the same code against a deployment holding real evidence would have been writing +customer text to the host since it shipped. + +## What LightRAG instruments, and what it does not + +The semantic port came from LightRAG, so its tracing was checked directly against +the pinned `v1.5.4` checkout rather than from recollection. An earlier reading in +this increment recorded that it has no observability at all; that was wrong. + +Its only instrumentation is Langfuse, and only for the OpenAI-compatible binding. +`lightrag/llm/openai.py:44-66` swaps `openai.AsyncOpenAI` for +`langfuse.openai.AsyncOpenAI` when `LANGFUSE_PUBLIC_KEY` and +`LANGFUSE_SECRET_KEY` are both present at import time. That drop-in captures the +full request and response, so enabling it exports prompts and completions +verbatim. `git grep -ln langfuse -- 'lightrag/llm/*.py'` returns that one file: +Anthropic, Gemini, Bedrock, Ollama and the rest are not traced at all. There is +no masking, redaction or scrubbing anywhere in the tree, no OpenTelemetry, no +metrics, and no spans over LightRAG's own pipeline stages. + +Two consequences. + +First, the upstream does precisely what the decision above rejects, at LLM-call +granularity, and reaches it through a vendor drop-in rather than a designed +boundary. It strengthens rather than weakens the finding that +"no comparable system chose never" describes an absence of deliberation, not a +considered industry position. It is not a precedent to adopt. + +Second, LightRAG is no source of stage coverage, because it instruments none. +Comparing the two pipelines instead surfaced a gap on this side: +`GraphRagEventSink.Stage` declares fourteen stages and production emits ten. +`PARSE`, `CHUNK`, `GLEAN` and `GENERATE` have no producer outside tests, so a +dashboard grouped by stage would show four permanently empty series and no +parsing, chunking, gleaning or answer-generation latency at all. Parsing and +chunking happen in the ingestion pipeline, which holds no sink; gleaning runs +inside extraction and is folded into it; generation is never reported, because +retrieval stops at `ASSEMBLE_CONTEXT`. + +Deletion and rebuild are absent from both the enum and the producers. LightRAG +tracks that path through `pipeline_status`, and the hardening runbook requires a +deletion-then-rebuild drill, so it is the one stage the comparison says is +missing outright rather than merely unwired. + +## Pipeline architecture + +- **Push over pull.** OTLP export is already wired and the worker has no HTTP + surface to scrape. Onyx needed a separate metrics server per Celery worker for + exactly that reason. Accepted cost: a push failure is invisible to the + monitoring system where a scrape failure is not; Compose healthchecks already + cover liveness. +- **One collector.** Alloy receives metrics and traces. Langfuse was rejected as + the primary destination because it does not accept JVM, HTTP, or database + metrics and would require a second pipeline. +- **Standard configuration.** `OTEL_EXPORTER_OTLP_ENDPOINT` maps to metrics, + traces, and logs through Boot's environment post-processor. OTLP log export is + disabled explicitly because Alloy already tails the `json-file` driver. +- **Silent when unconfigured.** No endpoint must mean no exporter and no + recurring log line. Tracing already behaves this way; metrics must match. +- **Readiness stays independent of exporter health.** + +## Trace completeness gaps + +- Worker jobs have no root span, so every span in an indexing job is an orphan + root correlated only by the `operation_id` attribute. +- `GraphRagKnowledgeRetrievalService:449` and `GraphIndexingProcessor:303` use + raw `Executors.newVirtualThreadPerTaskExecutor()`. Stage events are emitted on + the calling thread and keep their parent, but model calls inside those tasks + detach. `io.micrometer:context-propagation:1.2.1` is already on the classpath. +- `apps/mcp` has no OpenTelemetry starter, so an agent trace begins at the API + and its `RestClient` calls carry no `traceparent`. +- Resource attributes lack `service.version` and `deployment.environment`; + image tags already carry the commit SHA. +- Sampling is 0.1 for both API and worker. A low-volume batch workload should be + 1.0. + +## Exit proof + +- Production emits no recurring exporter warning. +- A provider failure path produces no prompt in any log stream. +- One retrieval and one indexing job appear in the collector as connected traces + with the expected stages, and a sanitized export shows no forbidden attribute. +- Stage latency is answerable from metrics at full coverage rather than from a + 10 % trace sample. diff --git a/docs/increments/active/2026-07-29-observability-pipeline/plan.md b/docs/increments/active/2026-07-29-observability-pipeline/plan.md new file mode 100644 index 00000000..66a0c928 --- /dev/null +++ b/docs/increments/active/2026-07-29-observability-pipeline/plan.md @@ -0,0 +1,127 @@ +# Observability pipeline plan + +## 0. Close the payload bypasses — code done, production evidence outstanding + +Highest priority. These are live paths, independent of the pipeline work. + +- [x] Pin `org.springframework.ai.openai` and `org.springframework.ai.anthropic` + logger levels to `ERROR`. Done in the base configuration of both apps + rather than the production profile, because a development database holds + real uploaded documents too, and without an environment override, because + the boundary is not a per-deployment setting. `ProviderLoggingBoundaryTests` + in each app reads the shipped YAML and fails if a profile lowers either + package. A fourth site was found during the sweep, + `AnthropicChatModel:1018`, which logs a response content block. +- [x] Search retained production logs for those WARN signatures. Not required: + the project owner confirmed on 2026-07-29 that the ZM deployment is a proof + of concept with no real users and no customer data, so there is no + historical exposure to scope and the fix is preventive rather than remedial. + If that ever stops being true before the fix ships to a deployment holding + real data, the search becomes a data-incident question again. +- [x] Decide how `OtelSpan.error` is handled. Suppressed, not tolerated: + `ExceptionSanitizingSpanExporter` drops every event attribute except + `exception.type` and clears the status description, as the last gate before + egress. The runbook's wording still needs the amendment in phase 5, because + the event itself now survives with its type. +- [x] Add a payload-free health signal for swallowed `GraphRagEventSink` + failures. `FailureTolerantGraphRagEventSink` counts them, records the + failure type and logs once per change of kind. Publishing that count as a + metric belongs with the Micrometer sink in phase 2; until then the signal + is the log line. + +## 1. Silence the unconfigured exporter — not started + +- [ ] `management.otlp.metrics.export.enabled` defaults to false; enabling it + requires an explicit endpoint. +- [ ] `management.logging.export.otlp.enabled: false` — Alloy tails `json-file`. +- [ ] `management.opentelemetry.map-environment-variables: false` in production + so host `OTEL_*` cannot enable export implicitly. +- [ ] Declare `spring.ai.chat.observations.include-error-logging: false` + explicitly rather than relying on the framework default. +- [ ] Add `service.version` and `deployment.environment` resource attributes. +- [ ] Worker tracing sampling to 1.0; API stays at 0.1. + +Gate: `compileJava`, `:core:test`. + +## 2. Metrics that answer stage latency — not started + +Depends on the composite sink merged in PR #132. + +- [ ] Remove `@ConditionalOnMissingBean(GraphRagEventSink.class)` from + `GraphRagObservabilityAutoConfiguration`; a sink is not an exclusive port. + Add a per-sink `@ConditionalOnProperty` toggle. +- [ ] Add `MicrometerGraphRagEventSink` — timers by stage, outcome and cache + status; counters by failure code. Metrics are not sampled, so stage p95 + becomes answerable at full coverage. +- [ ] Export `ContextTokenUsage`, which core already computes and nothing + publishes, plus a counter for `finish_reason=length` truncation. +- [ ] Time to first token on the streaming assistant path. +- [ ] Publish `FailureTolerantGraphRagEventSink.swallowedFailureCount()` as a + counter, so a broken sink is a number rather than only a log line. +- [ ] Close the stage gap the LightRAG comparison surfaced. `Stage` declares + fourteen and production emits ten: `PARSE` and `CHUNK` need a sink in the + ingestion pipeline, `GLEAN` needs separating from extraction, and + `GENERATE` needs emitting where retrieval currently stops at + `ASSEMBLE_CONTEXT`. Decide deletion and rebuild separately — it is missing + from the enum entirely and the runbook requires a drill for it. +- [ ] Tests: both sinks enabled receive the event; both disabled compose to + `NO_OP`. + +Gate: module tests, `:core:test`. + +## 3. Collector and dashboards — not started + +- [ ] `compose.observability.yaml` behind a profile, restoring the existing + Alloy/Loki/Tempo/Prometheus/Grafana stack. +- [ ] Join API and worker to the collector network; set + `OTEL_EXPORTER_OTLP_ENDPOINT`. Prometheus stays off the proxy network; + Grafana is published through Nginx Proxy Manager with Keycloak OIDC. +- [ ] Commit dashboards to the repository: infrastructure, and a separate AI + cost and quality board covering tokens by organization, truncation rate, + cache hit rate and TTFT. +- [ ] Extend `smoke-production.sh` to export a known signal and verify receipt, + so a silent exporter fails deployment instead of passing it. + +## 4. Trace continuity — not started + +Highest regression risk; run it last, once a collector can show the result. + +- [ ] Root span per worker indexing job. +- [ ] Wrap both virtual-thread executors with `ContextSnapshot` so model calls + inside tasks keep their parent. +- [ ] Add the OpenTelemetry starter to `apps/mcp` and propagate `traceparent` on + its API client. +- [ ] Prove: a job's child spans share the root `traceId`, and a span created + inside a virtual-thread task has the expected parent. + +## 5. Consolidate — not started + +- [ ] Whole-export test: scan every span, event, resource and + instrumentation-scope attribute, plus the log stream, on success and + failure paths, against the allowlist. This is the runbook's release gate + and it has never been executed. +- [ ] Reconcile `docs/specs/domains/secure-graph-rag.md` and its test matrix. +- [ ] Correct the runbook's field list — it omits `scopeFingerprint`, + `cacheStatus` and `occurredAt`, and describes counts as bounded when only + non-negativity is checked. +- [ ] Amend the runbook's "no exception event or stack trace" gate. The event now + survives carrying `exception.type` alone, which is what the exporter + enforces; the gate should say that rather than something stricter than the + code. +- [ ] Resolve where telemetry egress lives. `integrations/graph-rag-observability` + is named for one domain but now owns the span sanitizer, which protects + every span in the process. Renaming it is a module-boundary change and + needs its own challenge, so it is recorded here rather than done quietly. +- [ ] Record the payload-boundary decision under `docs/decisions/`, superseding + nothing but documenting the challenge outcome. + +## Deferred, with reasons + +- Prometheus scrape endpoint. OTLP push is already wired and the worker has no + scrape target. +- Langfuse as a destination. It accepts payload-free OTLP spans today with no + adapter code; it earns a pipeline only if governed content capture is ever + built. +- Closed `failureCode` taxonomy, keyed fingerprints, bucketed counts. Raised by + the architecture challenge as hardening beyond the current gap; worth doing, + not blocking. diff --git a/docs/roadmap.md b/docs/roadmap.md index ba3cd6df..ad4dffc7 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -40,6 +40,7 @@ The table is a delivery index, not a second description of current behavior. | [MCP search reliability](increments/active/2026-07-28-mcp-search-reliability/plan.md) | active | deploy merged timeout repair and prove the production MCP call | | [SCIM provisioning foundation](increments/active/2026-07-27-scim-provisioning-foundation/plan.md) | active | previous-binary/restore rehearsals and two-organization negative evidence | | [Public docs co-authoring and information architecture](increments/active/2026-07-29-public-docs-coauthoring/plan.md) | active | co-author What is OrgMemory? with the owner through context, outline, English review, teach-back, and Vietnamese review | +| [Observability pipeline and payload boundary](increments/active/2026-07-29-observability-pipeline/plan.md) | active | reach a collector, emit the four declared stages that have no producer, and prove a sanitized whole-export | The other SCIM directories under `increments/active/` are dependency-ordered future designs inside the active native identity program. They do not become diff --git a/docs/specs/domains/secure-graph-rag.md b/docs/specs/domains/secure-graph-rag.md index 9f6ece89..ab2a6cf3 100644 --- a/docs/specs/domains/secure-graph-rag.md +++ b/docs/specs/domains/secure-graph-rag.md @@ -5,7 +5,7 @@ Source: `components/graph-rag-core`, `components/graph-rag-testkit`, `core/src/main/java/com/orgmemory/core/knowledge`, and `apps/web/src/features/knowledge`. -Reconciled: `2026-07-29-graph-rag-observability-wiring (fd495d0)`. +Reconciled: `2026-07-29-observability-pipeline (f17256b)`. ## Current Contract @@ -140,10 +140,32 @@ Reconciled: `2026-07-29-graph-rag-observability-wiring (fd495d0)`. - Payload-free OpenTelemetry stages separate keyword planning/cache status, embedding, hashed per-snapshot retrieval, consolidation, authorization and provider-only reranking duration. +- `Stage` declares fourteen values; production emits ten. `PARSE`, `CHUNK`, + `GLEAN` and `GENERATE` have no producer, so no parsing, chunking, gleaning or + answer-generation latency is reported. Deletion and rebuild have no stage. - Indexing and retrieval fan one stage event out to every registered `GraphRagEventSink`, so an application may observe the same stage through more than one backend. Sinks fail independently and emission never controls - indexing or retrieval availability. + indexing or retrieval availability. Both composition sites wrap the composite + in `FailureTolerantGraphRagEventSink`, which absorbs an emission failure while + counting it, recording its type and logging once per change of kind, so a + backend broken since startup is distinguishable from a quiet one. Only class + names are recorded. +- Every exported span passes through `ExceptionSanitizingSpanExporter` before + leaving the process. It keeps `exception.type` alone from each event and clears + the status description, because Micrometer's bridge copies + `throwable.getMessage()` into both and an OrgMemory exception can be raised + holding query, evidence or provider-response text. It applies to all spans, not + only GraphRAG ones, and has no toggle. Span attributes are not filtered. +- The OpenAI and Anthropic client packages are pinned above WARN in both apps, + because their own logging concatenates the prompt or the response content into + messages that Spring AI's `log-prompt`/`log-completion` settings do not govern. + That pin is the default, not the boundary: `LOGGING_LEVEL_*` variables, system + properties, logback configuration and a level on a more specific logger all + outrank it. `ProviderLoggingBoundaryVerifier` asks each class holding such a + call site whether WARN is enabled on its own logger and fails startup if any + is, so the check is against the resolved level rather than any source of it. + It has no disable property. ## Graph Explorer diff --git a/docs/tests/domains/secure-graph-rag.md b/docs/tests/domains/secure-graph-rag.md index c2c742e1..5c074be5 100644 --- a/docs/tests/domains/secure-graph-rag.md +++ b/docs/tests/domains/secure-graph-rag.md @@ -6,7 +6,7 @@ Source: `components/graph-rag-core/src/test`, `core/src/test/java/com/orgmemory/core/knowledge`, and `apps/web/test/e2e`. -Reconciled: `2026-07-29-graph-rag-observability-wiring (fd495d0)`. +Reconciled: `2026-07-29-observability-pipeline (f17256b)`. ## Automated @@ -55,6 +55,29 @@ Reconciled: `2026-07-29-graph-rag-observability-wiring (fd495d0)`. failure, that the first failure is the one propagated with later ones suppressed, that two backends raising one shared failure instance do not trip self-suppression, and that an application with no backend emits nothing. +- Failure-tolerance tests prove the observed work still succeeds while the + backend is down, that every dropped event is counted, that a healthy backend + still receives the event and reports nothing, and that only the failure's class + name is retained when its message quotes the query it could not send. +- Span sanitization tests drive Micrometer's real `OtelSpan.error` through the + SDK and prove the exported span keeps `exception.type` and the error status + while losing the message, the stack trace and the status description, that the + stripped attributes still report as dropped rather than as never recorded, that + a successful span is untouched, and that an unmodelled event attribute does not + pass. Wiring tests prove the auto-configuration stays discoverable, wraps every + declared exporter, and is ordered ahead of Spring Boot's unwrapped collection. +- Provider-logging tests in each app read the shipped `application.yml` and fail + if the pin is absent, or if any `.yml`/`.yaml` profile lowers + `org.springframework.ai.openai` or `org.springframework.ai.anthropic` — or any + ancestor or descendant of them — back to WARN. They assert that at least one + profile file was scanned, so an unreadable resource directory cannot make them + pass vacuously. They cover the shipped default only. +- Boundary-verifier tests set levels the way an operator override would, against + the real provider classes and a real logging backend, and prove startup fails + when a leak site is at WARN or below, that a class set back to WARN is caught + even under a package pinned to ERROR, that the message names both the class and + the override to look for, that the auto-configuration stays discoverable, and + that the application context itself fails rather than starting. - Storage adapter auto-configuration tests prove PostgreSQL, OpenSearch and Neo4j stay discoverable through their registration files, that PostgreSQL owns the canonical ports without an opt-in, that OpenSearch and Neo4j claim no port @@ -87,6 +110,13 @@ The exact graph node/edge response and permission-negative metadata contract is covered at the API/service layer; a focused real-browser graph rendering and interaction test remains a gap. +No test asserts the whole telemetry export against the payload allowlist. Span +attributes, resource attributes, instrumentation-scope attributes and the log +stream are each unchecked; only the GraphRAG adapter's own attribute set, the +exception paths and the provider logger levels are covered. Until that test +exists, "payload-free" is enforced at the points listed above rather than proven +end to end. + `PostgresAuthorizedGraphSqlTests.graphVisibilityUsesTheLatestSealedCompleteAclAfterFreshnessExpiry` pins ADR 0015 parity: GraphRAG requires the current sealed `COMPLETE` ACL but does not add an expiry denial absent from canonical knowledge retrieval. diff --git a/integrations/graph-rag-observability/build.gradle.kts b/integrations/graph-rag-observability/build.gradle.kts index 2f45dc9d..288bf878 100644 --- a/integrations/graph-rag-observability/build.gradle.kts +++ b/integrations/graph-rag-observability/build.gradle.kts @@ -5,10 +5,23 @@ plugins { dependencies { api(project(":components:graph-rag-core")) implementation("io.opentelemetry:opentelemetry-api") + implementation("io.opentelemetry:opentelemetry-sdk-trace") + implementation("org.slf4j:slf4j-api") implementation("org.springframework.boot:spring-boot-autoconfigure") + // Supplies the SpanExporters collection this module replaces with a sanitized copy. + implementation("org.springframework.boot:spring-boot-micrometer-tracing-opentelemetry") testImplementation("io.opentelemetry:opentelemetry-sdk") testImplementation("io.opentelemetry:opentelemetry-sdk-testing") + // Drives the real bridge path this module sanitizes rather than a copy of it. + testImplementation("io.micrometer:micrometer-tracing-bridge-otel") testImplementation("org.junit.jupiter:junit-jupiter") + testImplementation("org.springframework.boot:spring-boot-test") + testImplementation("org.assertj:assertj-core") + // The logging boundary is only meaningful against the real leak sites and a real logging + // backend, so the verifier is tested against both rather than against stand-ins. + testImplementation("org.springframework.ai:spring-ai-openai") + testImplementation("org.springframework.ai:spring-ai-anthropic") + testImplementation("ch.qos.logback:logback-classic") testRuntimeOnly("org.junit.platform:junit-platform-launcher") } diff --git a/integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/ExceptionSanitizingSpanExporter.java b/integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/ExceptionSanitizingSpanExporter.java new file mode 100644 index 00000000..6c93aa66 --- /dev/null +++ b/integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/ExceptionSanitizingSpanExporter.java @@ -0,0 +1,136 @@ +package com.orgmemory.integrations.graphrag.observability; + +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.common.AttributesBuilder; +import io.opentelemetry.sdk.common.CompletableResultCode; +import io.opentelemetry.sdk.trace.data.DelegatingSpanData; +import io.opentelemetry.sdk.trace.data.EventData; +import io.opentelemetry.sdk.trace.data.SpanData; +import io.opentelemetry.sdk.trace.data.StatusData; +import io.opentelemetry.sdk.trace.export.SpanExporter; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Objects; +import java.util.Set; + +/** + * Removes exception text from every span on its way out of the process. + * + *

Micrometer's OpenTelemetry bridge ends an errored span by calling + * {@code recordException(throwable)} and {@code setStatus(ERROR, throwable.getMessage())}. + * Both carry an arbitrary message and a stack trace, and an OrgMemory exception can be + * raised while holding a query, an evidence chunk, a document identifier or a provider + * response body. Nothing upstream can prove otherwise, so the message and the stack trace + * are treated as payload and dropped. + * + *

The exception's type survives: it is a class name fixed by the source, it is the part + * an operator actually needs, and it cannot carry customer text. Attribute counts are left + * at their original values so a stripped attribute still shows up as dropped rather than as + * never recorded. + * + *

This exporter deliberately does not filter span attributes. Their allowlist is a wider + * question than exception handling, and pretending to cover it here would make a guarantee + * this class does not enforce. + * + *

Micrometer's {@code SpanFilter} is the natural hook for the events but cannot reach the + * status description, which {@code DelegatingSpanData} does not make mutable. Wrapping the + * exporter covers both, and it runs after every filter, so it is the last gate before egress. + */ +public final class ExceptionSanitizingSpanExporter implements SpanExporter { + + /** Event attributes that survive sanitization; everything else is treated as payload. */ + static final Set RETAINED_EVENT_ATTRIBUTES = Set.of("exception.type"); + + private final SpanExporter delegate; + + public ExceptionSanitizingSpanExporter(SpanExporter delegate) { + this.delegate = Objects.requireNonNull(delegate, "delegate"); + } + + @Override + public CompletableResultCode export(Collection spans) { + List sanitized = new ArrayList<>(spans.size()); + for (SpanData span : spans) { + sanitized.add(new SanitizedSpanData(span)); + } + return delegate.export(sanitized); + } + + @Override + public CompletableResultCode flush() { + return delegate.flush(); + } + + @Override + public CompletableResultCode shutdown() { + return delegate.shutdown(); + } + + @Override + public String toString() { + return "ExceptionSanitizingSpanExporter{" + delegate + "}"; + } + + private static final class SanitizedSpanData extends DelegatingSpanData { + + private final List events; + private final StatusData status; + + private SanitizedSpanData(SpanData delegate) { + super(delegate); + this.events = sanitizeEvents(delegate.getEvents()); + this.status = sanitizeStatus(delegate.getStatus()); + } + + @Override + public List getEvents() { + return events; + } + + @Override + public StatusData getStatus() { + return status; + } + + private static List sanitizeEvents(List events) { + List sanitized = new ArrayList<>(events.size()); + for (EventData event : events) { + sanitized.add(sanitizeEvent(event)); + } + return List.copyOf(sanitized); + } + + private static EventData sanitizeEvent(EventData event) { + Attributes retained = retainAllowedAttributes(event.getAttributes()); + if (retained.size() == event.getAttributes().size()) { + return event; + } + return EventData.create( + event.getEpochNanos(), + event.getName(), + retained, + event.getTotalAttributeCount()); + } + + private static Attributes retainAllowedAttributes(Attributes attributes) { + AttributesBuilder builder = Attributes.builder(); + attributes.forEach((key, value) -> { + if (RETAINED_EVENT_ATTRIBUTES.contains(key.getKey())) { + @SuppressWarnings("unchecked") + AttributeKey typedKey = (AttributeKey) key; + builder.put(typedKey, value); + } + }); + return builder.build(); + } + + private static StatusData sanitizeStatus(StatusData status) { + if (status.getDescription().isEmpty()) { + return status; + } + return StatusData.create(status.getStatusCode(), ""); + } + } +} diff --git a/integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/ProviderLoggingBoundaryAutoConfiguration.java b/integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/ProviderLoggingBoundaryAutoConfiguration.java new file mode 100644 index 00000000..f4bc88ea --- /dev/null +++ b/integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/ProviderLoggingBoundaryAutoConfiguration.java @@ -0,0 +1,22 @@ +package com.orgmemory.integrations.graphrag.observability; + +import org.springframework.beans.factory.InitializingBean; +import org.springframework.boot.autoconfigure.AutoConfiguration; +import org.springframework.context.annotation.Bean; + +/** + * Runs {@link ProviderLoggingBoundaryVerifier} during context refresh, so an application + * whose logging configuration would leak prompts fails to start instead of serving traffic. + * + *

Logging levels are applied long before refresh, and the actuator loggers endpoint is not + * exposed, so the level observed here is the one the provider libraries will see. There is no + * property to disable this: a boundary with an off switch is a default, not a boundary. + */ +@AutoConfiguration +public class ProviderLoggingBoundaryAutoConfiguration { + + @Bean + InitializingBean providerLoggingBoundaryVerification() { + return () -> ProviderLoggingBoundaryVerifier.verify(getClass().getClassLoader()); + } +} diff --git a/integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/ProviderLoggingBoundaryVerifier.java b/integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/ProviderLoggingBoundaryVerifier.java new file mode 100644 index 00000000..c42899a5 --- /dev/null +++ b/integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/ProviderLoggingBoundaryVerifier.java @@ -0,0 +1,55 @@ +package com.orgmemory.integrations.graphrag.observability; + +import java.util.List; +import org.slf4j.LoggerFactory; +import org.springframework.util.ClassUtils; + +/** + * Refuses to start an application whose configuration would let a provider library log a + * prompt. + * + *

The OpenAI and Anthropic clients build WARN messages by concatenating the prompt or the + * response content, guarded by {@code isWarnEnabled()}. Pinning those packages in + * {@code application.yml} makes the guard closed by default, but it is only a default: + * {@code LOGGING_LEVEL_ORG_SPRINGFRAMEWORK_AI_OPENAI}, a system property, a + * {@code logback.xml}, or a level set on a subordinate logger all outrank it. Configuration + * cannot enforce a boundary that configuration can undo. + * + *

So the check is made against the resolved state rather than against any source of it: + * each class that holds a payload-logging call site is asked whether WARN is enabled on its + * own logger, which is the exact question its guard will ask at runtime, with inheritance + * already applied. Startup fails rather than proceeds, because the alternative is an + * application that looks healthy while writing customer text to disk. + */ +final class ProviderLoggingBoundaryVerifier { + + /** + * Classes whose own logging concatenates prompt or response content into a WARN message. + * These are logger names, not merely packages: a level set on a child logger overrides an + * ancestor, so only the leak site's own logger answers the question its guard asks. + */ + static final List PAYLOAD_LOGGING_CLASSES = List.of( + "org.springframework.ai.openai.OpenAiChatModel", + "org.springframework.ai.openai.OpenAiAudioSpeechModel", + "org.springframework.ai.anthropic.AnthropicChatModel"); + + private ProviderLoggingBoundaryVerifier() { } + + static void verify(ClassLoader classLoader) { + List open = PAYLOAD_LOGGING_CLASSES.stream() + .filter(name -> ClassUtils.isPresent(name, classLoader)) + .filter(name -> LoggerFactory.getLogger(name).isWarnEnabled()) + .toList(); + if (open.isEmpty()) { + return; + } + throw new IllegalStateException( + "WARN logging is enabled for " + String.join(", ", open) + + ", which would write prompt and response content to the application log." + + " Those classes build the message only when WARN is enabled, so the level" + + " is the boundary. Something outranked the ERROR pin in application.yml:" + + " check LOGGING_LEVEL_* environment variables, -Dlogging.level.*, a" + + " logback configuration, and any level set on a more specific logger." + + " spring.ai.chat.observations.log-prompt does not govern these paths."); + } +} diff --git a/integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/SpanExportSanitizationAutoConfiguration.java b/integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/SpanExportSanitizationAutoConfiguration.java new file mode 100644 index 00000000..bb1fa880 --- /dev/null +++ b/integrations/graph-rag-observability/src/main/java/com/orgmemory/integrations/graphrag/observability/SpanExportSanitizationAutoConfiguration.java @@ -0,0 +1,31 @@ +package com.orgmemory.integrations.graphrag.observability; + +import io.opentelemetry.sdk.trace.export.SpanExporter; +import org.springframework.beans.factory.ObjectProvider; +import org.springframework.boot.autoconfigure.AutoConfiguration; +import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; +import org.springframework.boot.micrometer.tracing.opentelemetry.autoconfigure.SpanExporters; +import org.springframework.context.annotation.Bean; + +/** + * Puts {@link ExceptionSanitizingSpanExporter} in front of every span exporter. + * + *

Spring Boot contributes its own {@code SpanExporters} bean only when one is missing, so + * declaring this configuration ahead of it replaces the collection with a wrapped copy. Every + * exporter is covered, including ones added later, and there is no toggle: a payload guard + * that a deployment can switch off is not a guard. + */ +@AutoConfiguration( + beforeName = + "org.springframework.boot.micrometer.tracing.opentelemetry.autoconfigure" + + ".OpenTelemetryTracingAutoConfiguration") +@ConditionalOnClass({SpanExporters.class, SpanExporter.class}) +public class SpanExportSanitizationAutoConfiguration { + + @Bean + SpanExporters spanExporters(ObjectProvider spanExporters) { + return SpanExporters.of(spanExporters.orderedStream() + .map(ExceptionSanitizingSpanExporter::new) + .toList()); + } +} diff --git a/integrations/graph-rag-observability/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports b/integrations/graph-rag-observability/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports index 38153248..1355e59d 100644 --- a/integrations/graph-rag-observability/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports +++ b/integrations/graph-rag-observability/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports @@ -1 +1,3 @@ com.orgmemory.integrations.graphrag.observability.GraphRagObservabilityAutoConfiguration +com.orgmemory.integrations.graphrag.observability.ProviderLoggingBoundaryAutoConfiguration +com.orgmemory.integrations.graphrag.observability.SpanExportSanitizationAutoConfiguration diff --git a/integrations/graph-rag-observability/src/test/java/com/orgmemory/integrations/graphrag/observability/ExceptionSanitizingSpanExporterTests.java b/integrations/graph-rag-observability/src/test/java/com/orgmemory/integrations/graphrag/observability/ExceptionSanitizingSpanExporterTests.java new file mode 100644 index 00000000..58263d2f --- /dev/null +++ b/integrations/graph-rag-observability/src/test/java/com/orgmemory/integrations/graphrag/observability/ExceptionSanitizingSpanExporterTests.java @@ -0,0 +1,113 @@ +package com.orgmemory.integrations.graphrag.observability; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.micrometer.tracing.otel.bridge.OtelSpan; +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.trace.StatusCode; +import io.opentelemetry.api.trace.Tracer; +import io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter; +import io.opentelemetry.sdk.trace.SdkTracerProvider; +import io.opentelemetry.sdk.trace.data.EventData; +import io.opentelemetry.sdk.trace.data.SpanData; +import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor; +import java.util.function.Consumer; +import org.junit.jupiter.api.Test; + +/** + * The message under test is one an OrgMemory exception could realistically carry: a query and + * an evidence excerpt concatenated into a diagnostic string. If any assertion here regresses, + * that text reaches whatever collector the deployment points at. + */ +class ExceptionSanitizingSpanExporterTests { + + private static final String PAYLOAD = + "no evidence for query 'quarterly revenue for ACME' in chunk 'ACME booked 4.2M in Q3'"; + + private static final AttributeKey EXCEPTION_TYPE = AttributeKey.stringKey("exception.type"); + private static final AttributeKey EXCEPTION_MESSAGE = AttributeKey.stringKey("exception.message"); + private static final AttributeKey EXCEPTION_STACKTRACE = + AttributeKey.stringKey("exception.stacktrace"); + + @Test + void stripsTheMessageAndStackTraceMicrometerRecordsWhenASpanFails() { + SpanData exported = export(span -> new OtelSpan(span).error(new IllegalStateException(PAYLOAD))); + + EventData exception = onlyEvent(exported); + assertEquals("exception", exception.getName()); + assertNull(exception.getAttributes().get(EXCEPTION_MESSAGE), "the exception message is payload"); + assertNull(exception.getAttributes().get(EXCEPTION_STACKTRACE), "the stack trace is payload"); + } + + @Test + void keepsTheExceptionTypeBecauseAClassNameCannotCarryCustomerText() { + SpanData exported = export(span -> new OtelSpan(span).error(new IllegalStateException(PAYLOAD))); + + assertEquals( + IllegalStateException.class.getName(), + onlyEvent(exported).getAttributes().get(EXCEPTION_TYPE)); + } + + @Test + void clearsTheStatusDescriptionMicrometerCopiesFromTheExceptionMessage() { + SpanData exported = export(span -> new OtelSpan(span).error(new IllegalStateException(PAYLOAD))); + + assertEquals(StatusCode.ERROR, exported.getStatus().getStatusCode(), "the failure itself must survive"); + assertEquals("", exported.getStatus().getDescription()); + } + + @Test + void leavesTheStrippedAttributesVisibleAsDroppedRatherThanNeverRecorded() { + SpanData exported = export(span -> new OtelSpan(span).error(new IllegalStateException(PAYLOAD))); + + EventData exception = onlyEvent(exported); + assertEquals(3, exception.getTotalAttributeCount(), "the original count is the honest one"); + assertEquals(2, exception.getDroppedAttributesCount()); + } + + @Test + void leavesASuccessfulSpanExactlyAsItWas() { + SpanData exported = export(span -> span.setAttribute("orgmemory.graph_rag.stage", "retrieve")); + + assertTrue(exported.getEvents().isEmpty()); + assertEquals("", exported.getStatus().getDescription()); + assertEquals( + "retrieve", + exported.getAttributes().get(AttributeKey.stringKey("orgmemory.graph_rag.stage")), + "sanitization must not disturb the payload-free attributes the pipeline exists to carry"); + } + + @Test + void refusesToLetAnUnmodelledEventAttributeThroughUninspected() { + SpanData exported = export(span -> span.addEvent( + "orgmemory.diagnostic", + io.opentelemetry.api.common.Attributes.of(AttributeKey.stringKey("note"), PAYLOAD))); + + EventData event = onlyEvent(exported); + assertEquals("orgmemory.diagnostic", event.getName(), "event names are code-derived and survive"); + assertNull(event.getAttributes().get(AttributeKey.stringKey("note"))); + } + + /** Runs one span through the real SDK and the exporter, and returns what the collector would see. */ + private static SpanData export(Consumer work) { + InMemorySpanExporter collector = InMemorySpanExporter.create(); + try (SdkTracerProvider provider = SdkTracerProvider.builder() + .addSpanProcessor(SimpleSpanProcessor.create(new ExceptionSanitizingSpanExporter(collector))) + .build()) { + Tracer tracer = provider.get("test"); + io.opentelemetry.api.trace.Span span = tracer.spanBuilder("test").startSpan(); + work.accept(span); + span.end(); + return collector.getFinishedSpanItems().getFirst(); + } + } + + private static EventData onlyEvent(SpanData span) { + assertFalse(span.getEvents().isEmpty(), "the event itself must survive so the failure stays visible"); + assertEquals(1, span.getEvents().size()); + return span.getEvents().getFirst(); + } +} diff --git a/integrations/graph-rag-observability/src/test/java/com/orgmemory/integrations/graphrag/observability/ProviderLoggingBoundaryVerifierTests.java b/integrations/graph-rag-observability/src/test/java/com/orgmemory/integrations/graphrag/observability/ProviderLoggingBoundaryVerifierTests.java new file mode 100644 index 00000000..a0e2ced4 --- /dev/null +++ b/integrations/graph-rag-observability/src/test/java/com/orgmemory/integrations/graphrag/observability/ProviderLoggingBoundaryVerifierTests.java @@ -0,0 +1,132 @@ +package com.orgmemory.integrations.graphrag.observability; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.LoggerContext; +import java.util.ArrayList; +import java.util.List; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; +import org.springframework.boot.autoconfigure.AutoConfiguration; +import org.springframework.boot.autoconfigure.AutoConfigurations; +import org.springframework.boot.context.annotation.ImportCandidates; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; + +/** + * The YAML pin is a default, not a boundary: an environment variable, a system property, a + * logback configuration or a level on a more specific logger all outrank it. This verifier + * exists to catch that, so the tests set the level the way an operator's override would and + * check the resolved state rather than the configuration that produced it. + */ +class ProviderLoggingBoundaryVerifierTests { + + private static final String LEAK_SITE = "org.springframework.ai.openai.OpenAiChatModel"; + + private final List touched = new ArrayList<>(); + + /** + * The test JVM has no logging configuration, so every leak site starts WARN-enabled. Each + * test begins from the closed state the shipped YAML produces and then reopens exactly one, + * so a failure names the site the test is about rather than the default. + */ + @org.junit.jupiter.api.BeforeEach + void closeEveryLeakSite() { + ProviderLoggingBoundaryVerifier.PAYLOAD_LOGGING_CLASSES + .forEach(name -> setLevel(name, Level.ERROR)); + } + + @AfterEach + void restoreLevels() { + touched.forEach(name -> loggerContext().getLogger(name).setLevel(null)); + touched.clear(); + } + + @Test + void startsWhenEveryLeakSiteIsAboveWarn() { + assertDoesNotThrow(() -> ProviderLoggingBoundaryVerifier.verify(classLoader())); + } + + @Test + void refusesToStartWhenAnOverrideReopensAProviderLeakSite() { + setLevel(LEAK_SITE, Level.WARN); + + IllegalStateException failure = assertThrows( + IllegalStateException.class, + () -> ProviderLoggingBoundaryVerifier.verify(classLoader())); + + assertTrue(failure.getMessage().contains(LEAK_SITE), "the message must name what to fix"); + assertTrue( + failure.getMessage().contains("LOGGING_LEVEL"), + "the message must point at the override that outranked the YAML pin"); + } + + @Test + void guardsEveryClassKnownToConcatenatePayloadIntoAWarnMessage() { + assertTrue( + ProviderLoggingBoundaryVerifier.PAYLOAD_LOGGING_CLASSES.containsAll(List.of( + "org.springframework.ai.openai.OpenAiChatModel", + "org.springframework.ai.anthropic.AnthropicChatModel")), + "dropping a known leak site from the list silently reopens it"); + } + + @Test + void refusesToStartWhenTheLevelIsLoweredBelowWarnRatherThanToIt() { + setLevel(LEAK_SITE, Level.DEBUG); + + assertThrows( + IllegalStateException.class, + () -> ProviderLoggingBoundaryVerifier.verify(classLoader())); + } + + @Test + void checksTheLeakSiteRatherThanItsPackageSoAChildOverrideCannotHide() { + setLevel("org.springframework.ai.openai", Level.ERROR); + setLevel(LEAK_SITE, Level.WARN); + + assertThrows( + IllegalStateException.class, + () -> ProviderLoggingBoundaryVerifier.verify(classLoader()), + "a package pinned to ERROR must not excuse a class set back to WARN"); + } + + @Test + void staysDiscoverableWithoutAnApplicationNamingIt() { + List names = new ArrayList<>(); + ImportCandidates.load(AutoConfiguration.class, classLoader()).forEach(names::add); + + assertTrue( + names.contains(ProviderLoggingBoundaryAutoConfiguration.class.getName()), + "META-INF/spring/…AutoConfiguration.imports no longer names this class, so an " + + "application could start with prompt logging enabled"); + } + + @Test + void failsTheApplicationContextRatherThanLettingItServeTraffic() { + setLevel(LEAK_SITE, Level.WARN); + + new ApplicationContextRunner() + .withConfiguration( + AutoConfigurations.of(ProviderLoggingBoundaryAutoConfiguration.class)) + .run(context -> assertTrue( + context.getStartupFailure() != null, + "startup must fail; an application that looks healthy while logging " + + "prompts is the outcome this guard exists to prevent")); + } + + private void setLevel(String logger, Level level) { + touched.add(logger); + loggerContext().getLogger(logger).setLevel(level); + } + + private static LoggerContext loggerContext() { + return (LoggerContext) LoggerFactory.getILoggerFactory(); + } + + private static ClassLoader classLoader() { + return ProviderLoggingBoundaryVerifierTests.class.getClassLoader(); + } +} diff --git a/integrations/graph-rag-observability/src/test/java/com/orgmemory/integrations/graphrag/observability/SpanExportSanitizationAutoConfigurationTests.java b/integrations/graph-rag-observability/src/test/java/com/orgmemory/integrations/graphrag/observability/SpanExportSanitizationAutoConfigurationTests.java new file mode 100644 index 00000000..2f4e2522 --- /dev/null +++ b/integrations/graph-rag-observability/src/test/java/com/orgmemory/integrations/graphrag/observability/SpanExportSanitizationAutoConfigurationTests.java @@ -0,0 +1,87 @@ +package com.orgmemory.integrations.graphrag.observability; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter; +import io.opentelemetry.sdk.trace.export.SpanExporter; +import java.util.ArrayList; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.springframework.boot.autoconfigure.AutoConfiguration; +import org.springframework.boot.autoconfigure.AutoConfigurations; +import org.springframework.boot.context.annotation.ImportCandidates; +import org.springframework.boot.micrometer.tracing.opentelemetry.autoconfigure.OpenTelemetryTracingAutoConfiguration; +import org.springframework.boot.micrometer.tracing.opentelemetry.autoconfigure.SpanExporters; +import org.springframework.boot.opentelemetry.autoconfigure.OpenTelemetrySdkAutoConfiguration; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +/** + * The sanitizer only protects anything if it is actually in the export path. Spring Boot + * contributes its own unwrapped {@code SpanExporters} whenever one is missing, so the two + * questions are whether this module is discovered at all and whether it is ordered ahead of + * Boot's copy. + */ +class SpanExportSanitizationAutoConfigurationTests { + + private final ApplicationContextRunner runner = new ApplicationContextRunner() + .withConfiguration(AutoConfigurations.of(SpanExportSanitizationAutoConfiguration.class)) + .withUserConfiguration(ExporterConfiguration.class); + + @Test + void staysDiscoverableWithoutAnApplicationNamingIt() { + assertTrue( + registeredAutoConfigurations() + .contains(SpanExportSanitizationAutoConfiguration.class.getName()), + "META-INF/spring/…AutoConfiguration.imports no longer names this class, so spans " + + "would reach the collector with their exception messages intact"); + } + + @Test + void wrapsEveryExporterTheApplicationDeclares() { + runner.run(context -> { + List exporters = context.getBean(SpanExporters.class).list(); + assertEquals(2, exporters.size(), "no exporter may be dropped or left unwrapped"); + exporters.forEach(exporter -> assertInstanceOf(ExceptionSanitizingSpanExporter.class, exporter)); + }); + } + + @Test + void winsOverSpringBootsUnwrappedCollection() { + runner.withConfiguration(AutoConfigurations.of( + OpenTelemetrySdkAutoConfiguration.class, + OpenTelemetryTracingAutoConfiguration.class)) + .run(context -> context.getBean(SpanExporters.class) + .list() + .forEach(exporter -> assertInstanceOf( + ExceptionSanitizingSpanExporter.class, + exporter, + "the beforeName ordering no longer beats Boot's own SpanExporters bean"))); + } + + private static List registeredAutoConfigurations() { + List names = new ArrayList<>(); + ImportCandidates.load( + AutoConfiguration.class, + SpanExportSanitizationAutoConfigurationTests.class.getClassLoader()) + .forEach(names::add); + return names; + } + + @Configuration(proxyBeanMethods = false) + static class ExporterConfiguration { + + @Bean + SpanExporter firstExporter() { + return InMemorySpanExporter.create(); + } + + @Bean + SpanExporter secondExporter() { + return InMemorySpanExporter.create(); + } + } +}