fix(maf): prevent metric inflation and context breaks#244
Conversation
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Fixes two MAF observability regressions: (1) removes process-cumulative ObservableGauge callbacks from MAFSemanticProcessor that caused GenAI metric inflation, routing MAF's native metric instruments to explicitly supplied providers via util_genai_bridge; (2) adds a generic existing-span lifecycle API (use_existing_span, mark_first_token, record_existing_span) to opentelemetry-util-genai so MAF-owned AGENT spans remain current without transferring span ownership or ending native spans. Includes thread-safe locking and fork-safe lock reinitialization.
Overall verdict: looks good. Well-structured, comprehensive test coverage (thread safety, fork safety, legacy constructor compat, streaming, concurrent finalization, error paths). The architectural change of moving metrics aggregation out of the span processor and into the util-genai handler is the right approach — it eliminates the ObservableGauge inflation at the source.
Posting as COMMENT rather than APPROVE because CI is still pending (PR was pushed ~20 min ago). Will approve once CI passes.
Findings
- [Info]
extended_handler.py:223—isinstanceguard replaced withcast()(duck typing); subtle behavior change for custom recorders - [Info]
__init__.py:136— Good defensive rollback onadd_span_processorfailure
Suggestions
- Consider documenting the
record_extendedsignature change (record_standard_metricsparameter) and theisinstance→castswitch as a breaking change for customExtendedInvocationMetricsRecordersubclasses in the changelog, since downstream instrumentations that vendor a custom recorder could be affected.
Cross-repo Note
Changes to util/opentelemetry-util-genai (existing-span lifecycle API, record_extended signature, _prepare_model_context hook) may affect downstream instrumentation plugins that depend on this util package. The related repos (loongsuite-pilot, loongcollector) don't directly import the Python util, so no immediate cross-repo action needed.
Automated review by github-manager-bot
| # BaseInstrumentor does not mark the instrumentor active when | ||
| # _instrument raises, so a later uninstrument cannot clean up a | ||
| # bridge installed above. Roll back this partial transaction now. | ||
| revert_util_genai_bridge() |
There was a problem hiding this comment.
[Info] Good defensive rollback — if add_span_processor fails, calling revert_util_genai_bridge() and _remove_span_processor() prevents leaving the instrumentation in a partial state where the bridge is installed but the processor is not. Solid error handling.
6028e07 to
fd2cd94
Compare
ralf0131
left a comment
There was a problem hiding this comment.
Summary
MAF instrumentation fix for metric inflation and context breaks. Scope narrowed to MAF-only since the previous revision (shared util/opentelemetry-util-genai changes removed). CI now passes on all core quality checks.
Previous review (commit 6028e076) was COMMENT-only pending CI; my prior findings about the util/opentelemetry-util-genai existing-span lifecycle API no longer apply since those changes were removed. The remaining MAF-only changes are sound:
- Removing the process-cumulative
ObservableGaugecallbacks eliminates metric inflation at the source — MAF's native counter/histogram instruments remain the single metric source. - Routing through explicitly supplied
tracer_provider/meter_providerviautil_genai_bridgerestores correct parent/child trace relationships. - Non-streaming span activation + optional Robin SDK suppression is properly guarded (
ImportErrorfallback) and scoped to the active LLM call. - Rollback via
revert_util_genai_bridge()onadd_span_processorfailure is a good defensive measure. - Test coverage is comprehensive (no-gauge registration, rollback, span context + suppression, provider routing).
LGTM — approving now that CI passes and the scope is contained to the MAF package.
Automated review by github-manager-bot
Description
This PR fixes two Microsoft Agent Framework observability regressions within the MAF instrumentation only:
ObservableGaugecallbacks fromMAFSemanticProcessor, allowing MAF's native counter/histogram instruments to remain the single metric source;When the optional Robin SDK extension is present, the bridge also scopes its LLM SDK suppression key to the active MAF LLM call so the underlying SDK does not emit a second LLM span.
The shared
util/opentelemetry-util-genaipackage is intentionally unchanged. Existing MAF constructor/config compatibility surfaces remain available, but no longer register or emit the removed cumulative gauges.Fixes # (N/A)
Type of change
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist
Validation Evidence
Scope
loongsuite-instrumentation-microsoft-agent-frameworkonly.git diff origin/main...HEAD -- util/opentelemetry-util-genaiis empty).|loongsuite-instrumentation-microsoft-agent-framework|; not degraded and not full-repo.Fix Verification
The same isolated repro was run on merge base
be4f7d3and on this branch:ObservableGaugeregistrationsLocal Checks
python "$PIPELINE_SKILL_DIR/scripts/check_loongsuite_pr_readiness.py" --repo .PRE_COMMIT_HOME="$(mktemp -d /tmp/loongsuite-precommit.XXXXXX)" tox -e precommittox -e typechecktox -c tox-loongsuite.ini -e py312-test-loongsuite-instrumentation-microsoft-agent-frameworktox -c tox-loongsuite.ini -e py312-test-loongsuite-instrumentation-microsoft-agent-framework-oldesttox -c tox-loongsuite.ini -e lint-loongsuite-instrumentation-microsoft-agent-frameworkgit diff --check origin/main...HEADReal E2E Matrix
AGENT -> STEP -> LLM + TOOLThe cloud readback found all 9 scenario trace IDs and all 39 emitted spans. The canonical GenAI trace validator passed all 7 GenAI spans in the two-tool ReAct trace.
Metrics
The live SDK run emitted only MAF's native metrics:
gen_ai.client.operation.durationgen_ai.client.token.usageagent_framework.function.invocation.durationNo legacy
genai_calls_*orgenai_llm_usage_tokensSDK gauge was created. The backend-derived call metric readback contained exactly 30 GenAI calls, matching the 30 GenAI spans in the trace readback, and had only the backendxtracesource.Telemetry Contract
NO_CONTENTSPAN_ONLYSPAN_AND_EVENTgen_ai.*attributes were recognized; remaining violations are existing registry gaps for standard resource attributes and native MAF tool-duration attributesExisting CI Context
The current remote checks belong to the previous broader commit that also changed the shared util package. That scope caused the package detector to run all util consumers. The Google ADK failures are missing
orjson, and the LiteLLM failures are missingfastapi; both reproduce as dependency-environment failures and are unrelated to the MAF change. After this MAF-only revision is pushed, the detector selects only the MAF package.