Skip to content

feat(weather-service): add MLflow autolog observability support#223

Merged
pdettori merged 1 commit intokagenti:mainfrom
Ygnas:feat/weather-service-mlflow-observability
Apr 9, 2026
Merged

feat(weather-service): add MLflow autolog observability support#223
pdettori merged 1 commit intokagenti:mainfrom
Ygnas:feat/weather-service-mlflow-observability

Conversation

@Ygnas
Copy link
Copy Markdown
Contributor

@Ygnas Ygnas commented Apr 9, 2026

Summary

Auto-detect tracing backend from environment variables: MLflow
LangChain autolog when MLFLOW_TRACKING_URI is set, OpenTelemetry with
OpenInference when OTEL_EXPORTER_OTLP_ENDPOINT is set. When both are
configured, both backends are active simultaneously and
MLFLOW_TRACE_ENABLE_OTLP_DUAL_EXPORT is enabled automatically so
traces reach both MLflow Tracking Server and OTEL Collector.

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

Related issue(s)

(Optional) Testing Instructions

Fixes #

Copy link
Copy Markdown
Contributor

@pdettori pdettori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Clean, well-structured change that adds MLflow autolog support with good env-var-based auto-detection and graceful degradation. The main concern is the silent removal of the default OTLP endpoint, which is a breaking change for existing deployments that relied on it.

Areas reviewed: Python, Dependencies, Security (Trivy)
Commits: 1 commit, signed-off: ✅
CI status: Trivy failing (transitive dep CVEs in aiohttp, marshmallow, cryptography, requests — not directly from this PR's code), all other checks pass

Missing tests: No tests added for the new MLflow autolog path or dual-export logic. Even a unit test mocking mlflow.langchain.autolog to verify it's called when MLFLOW_TRACKING_URI is set would add confidence.

Verdict: COMMENT — Item below (breaking change on default endpoint) should be addressed or acknowledged.

_root_span_var: ContextVar = ContextVar("root_span", default=None)

# Module-level flag set by setup_observability() to indicate active backend
_use_otel = False
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit — Module-level mutable global is fine for init-once, but if you ever want to test the different code paths in isolation, a simple namespace (e.g., _state.use_otel) would make mocking cleaner.

@Ygnas Ygnas force-pushed the feat/weather-service-mlflow-observability branch from b5879ef to a598cd0 Compare April 9, 2026 19:15
@Ygnas Ygnas requested a review from pdettori April 9, 2026 19:16
Auto-detect tracing backend from environment variables: MLflow
LangChain autolog when MLFLOW_TRACKING_URI is set, OpenTelemetry with
OpenInference when OTEL_EXPORTER_OTLP_ENDPOINT is set. When both are
configured, both backends are active simultaneously and
MLFLOW_TRACE_ENABLE_OTLP_DUAL_EXPORT is enabled automatically so
traces reach both MLflow Tracking Server and OTEL Collector.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com>
@Ygnas Ygnas force-pushed the feat/weather-service-mlflow-observability branch from a598cd0 to 6b1d81e Compare April 9, 2026 19:20
@pdettori
Copy link
Copy Markdown
Contributor

pdettori commented Apr 9, 2026

Trivy CI Failure Analysis

The Trivy check failure is not caused by this PR. All 16 CVEs are in transitive dependencies that already exist on main at the exact same versions:

Package Version (main = PR) CVEs Fix available
aiohttp 3.13.3 8 3.13.4
marshmallow 3.26.1 1 3.26.2
cryptography 46.0.5 2 46.0.6 / 46.0.7
requests 2.32.5 1 2.33.0
jwcrypto 1.5.6 1 none yet
langchain-core 1.2.22 1 0.3.x (likely false positive on version range)

Because the PR regenerated uv.lock (+1245 lines), Trivy's code-scanning treated the full lockfile as "changed" and surfaced pre-existing CVEs. The Trivy summary itself notes: "Alerts not introduced by this pull request might have been detected because the code changes were too large."

Bumping these deps (especially aiohttp → 3.13.4) would be good housekeeping but is independent of this PR.

Copy link
Copy Markdown
Contributor

@pdettori pdettori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@pdettori pdettori merged commit 0254eab into kagenti:main Apr 9, 2026
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants