feat(http): add client instrumentation and metrics - #2219
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
933c19b to
b77d54f
Compare
PR merge guidance@Pouyanpi thanks for the PR. GitHub is currently blocking merge for one or more repository requirements:
Relevant guide: |
Greptile SummaryAdds optional tracing and request-duration metrics around the shared HTTP client.
|
| Filename | Overview |
|---|---|
| nemoguardrails/http/instrumented.py | Adds the telemetry decorator and retryable close handling, but concurrent cleanup can report false success when the active close fails. |
| nemoguardrails/http/telemetry.py | Adds privacy-conscious span attributes, error events, and HTTP request-duration recording. |
| nemoguardrails/tracing/constants.py | Adds HTTP semantic attributes and lazily initialized request-duration histogram infrastructure. |
| tests/http/test_instrumentation.py | Covers tracing, metrics, passthrough behavior, idempotent instrumentation, and successful concurrent close behavior. |
| nemoguardrails/http/init.py | Exports the new instrumented client and helper through the public HTTP package surface. |
Sequence Diagram
sequenceDiagram
participant A as Close caller A
participant I as InstrumentedHTTPClient
participant B as Close caller B
participant W as Wrapped client
A->>I: close()
I->>I: "_closed = true"
I->>W: await close()
B->>I: close()
I-->>B: return normally
W-->>I: raises or is cancelled
I->>I: "_closed = false"
I-->>A: re-raise
Note over B,W: B observed success although cleanup failed
Prompt To Fix All With AI
### Issue 1
nemoguardrails/http/instrumented.py:123-124
**Concurrent close reports false success**
When two callers invoke `close()` concurrently and the wrapped close is cancelled or raises, the second caller returns normally while cleanup is still in progress. The first caller then resets `_closed` and propagates the failure, leaving the second caller to treat uncompleted cleanup as successful and potentially discard a client whose underlying connections remain open.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (3): Last reviewed commit: "docs(http): document client telemetry su..." | Re-trigger Greptile
📝 WalkthroughWalkthroughAdds OpenTelemetry tracing and request-duration metrics for HTTP clients, including semantic attributes, error and retry telemetry, idempotent client wrapping, public exports, and comprehensive asynchronous tests. ChangesHTTP instrumentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant instrument_http_client
participant InstrumentedHTTPClient
participant HTTPClient
participant OpenTelemetry
Caller->>instrument_http_client: request instrumentation
instrument_http_client->>InstrumentedHTTPClient: create or reuse wrapper
InstrumentedHTTPClient->>OpenTelemetry: start span and duration measurement
InstrumentedHTTPClient->>HTTPClient: forward HTTP request
HTTPClient-->>InstrumentedHTTPClient: return HTTPResponse
InstrumentedHTTPClient->>OpenTelemetry: record response attributes and metrics
InstrumentedHTTPClient-->>Caller: return HTTPResponse
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Pouyan <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
| if self._closed: | ||
| return |
There was a problem hiding this comment.
Concurrent close reports false success
When two callers invoke close() concurrently and the wrapped close is cancelled or raises, the second caller returns normally while cleanup is still in progress. The first caller then resets _closed and propagates the failure, leaving the second caller to treat uncompleted cleanup as successful and potentially discard a client whose underlying connections remain open.
Knowledge Base Used: Outbound HTTP Foundation
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/http/instrumented.py
Line: 123-124
Comment:
**Concurrent close reports false success**
When two callers invoke `close()` concurrently and the wrapped close is cancelled or raises, the second caller returns normally while cleanup is still in progress. The first caller then resets `_closed` and propagates the failure, leaving the second caller to treat uncompleted cleanup as successful and potentially discard a client whose underlying connections remain open.
**Knowledge Base Used:** [Outbound HTTP Foundation](https://app.greptile.com/nvidia-public-github/-/custom-context/knowledge-base/nvidia-nemo/guardrails/-/docs/http-foundation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Description
Adds privacy-safe tracing and request-duration metrics as a decorator over the
shared HTTP client contract introduced in Stack 9.
Design principle: instrumentation observes the client boundary without owning
request or client lifecycle.
http_call, IORails, and other callers can keepusing the same
HTTPClientcontract, while tracing and metrics remain optionaland independently configurable.
Related Issue(s)
AI Assistance
Codex assisted with implementation, validation, and stack restructuring. Check
the disclosure box after human review.
Checklist
Summary by CodeRabbit