adityamparikh commented on PR #23: URL: https://github.com/apache/solr-mcp/pull/23#issuecomment-5358193658
Pushed 6364bd3 addressing review findings. ## Versions bumped **4.1.0 → 4.1.1** and **2.0.0 → 2.0.1**, both checked against `maven-metadata.xml` directly (the Maven Central search index returns stale, unsorted results for these coordinates). The PR description called 4.1.0/2.0.0 "the latest GA releases" — true when written, no longer. Description updated to match. ## The substantive finding: observability docs would have broken users This PR moves OTLP export from **gRPC on 4317** to **HTTP on 4318**, with a separate full URL per signal. That silently redefines `OTEL_TRACES_URL` — it used to be a *base* endpoint, and is now a *complete* path ending in `/v1/traces`. `docs/observability.md` — the user-facing guide README links to — was untouched by this PR and still documented the removed `otel.exporter.otlp.*` properties plus: ```bash export OTEL_TRACES_URL=https://otel-collector.example.com:4317 ``` Following that after this merged would **not error**. Traces would just stop arriving, which is the worst way for a telemetry config to fail. Fixed: - `docs/observability.md` — real property names, corrected production example, a variable table, and an explicit callout that the semantics changed for anyone upgrading - `dev-docs/Observability.md` — this PR *adds* it, so it now states its relationship to the user-facing guide rather than silently competing with it; the three OTLP defaults are filled in instead of "(auto-configured)" - `README.md` — names all four OTEL variables - `AGENTS.md` — version references, plus the OTEL variables in Key Configuration Also worth flagging as **breaking** in the release notes, not just the docs: anyone with `OTEL_TRACES_URL` already set in a deployment is affected. ## Smaller items - **Broken image removed.** `dev-docs/Observability.md` referenced `images/grafana-traces.png`, which does not exist anywhere in the repo — a broken-image icon on GitHub, introduced by this PR. The prose following it already describes the trace breakdown span by span, so it stands alone. Caught by a link check, not by reading. - **The mongodb exclude is now explained.** It sat uncommented in a file where the cyclonedx pin above it gets fourteen lines of rationale — exactly the kind of thing someone deletes while tidying, and being `developmentOnly` it would break local `bootRun` without CI noticing. - **CycloneDX pin now has a tracker** (#186) linked from both `build.gradle.kts` and `AGENTS.md`, with a checklist for verifying the SBOM still lands where `generateBinaryLicense` expects it. ## Verification - `./gradlew build` — green, **372 tests / 0 failures**, matching `main`'s count exactly - `./gradlew nativeTest -Pnative` — green, **227 passing / 0 failed**, unchanged by the bump ## Merge-order note This PR and #175 both rewrite the same line in `JsonResponseParser.toNamedList` — this one changes `fields().forEachRemaining(...)` to `properties().forEach(...)` for Jackson 3, #175 threads a node path through it for the empty-facet fix. They compose (the `properties()` call *plus* the path argument), but it is a guaranteed conflict. #175 is small and ready, so merging it first makes this the cheaper rebase. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
