Skip to content

feat(http): add client instrumentation and metrics - #2219

Open
Pouyanpi wants to merge 5 commits into
developfrom
pouyanpi/rail-library-stack-10-http-instrumentation
Open

feat(http): add client instrumentation and metrics#2219
Pouyanpi wants to merge 5 commits into
developfrom
pouyanpi/rail-library-stack-10-http-instrumentation

Conversation

@Pouyanpi

@Pouyanpi Pouyanpi commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

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 keep
using the same HTTPClient contract, while tracing and metrics remain optional
and independently configurable.

Related Issue(s)

  • Internal tracking: NGUARD-857, NGUARD-860

AI Assistance

  • No AI tools were used.
  • AI tools were used; a human reviewed and can explain every change (tool: Codex).

Codex assisted with implementation, validation, and stack restructuring. Check
the disclosure box after human review.

Checklist

  • I've read the CONTRIBUTING guidelines.
  • This PR links to a triaged issue assigned to me.
  • My PR title follows the project commit convention.
  • I've updated the documentation if applicable.
  • I've added tests if applicable.
  • I've noted any verification beyond CI and any checks I couldn't run.
  • I did not update generated changelog files manually.
  • I addressed all CodeRabbit, Greptile, and other review comments, or replied with why no change is needed.
  • @mentions of the person or team responsible for reviewing proposed changes.

Summary by CodeRabbit

  • New Features
    • Added optional OpenTelemetry instrumentation for outgoing HTTP requests.
    • HTTP traces now include request, response, retry, error, and duration details while excluding sensitive values.
    • Added HTTP request-duration metrics with status and error information.
    • Instrumentation can be enabled through the public HTTP client interface and safely avoids duplicate wrapping.

@github-actions github-actions Bot added status: needs triage New issues that have not yet been reviewed or categorized. size: L labels Jul 24, 2026
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.43243% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
nemoguardrails/http/telemetry.py 90.56% 10 Missing ⚠️
nemoguardrails/http/instrumented.py 94.23% 3 Missing ⚠️
nemoguardrails/tracing/constants.py 96.15% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Base automatically changed from pouyanpi/rail-library-stack-9-http-contract to develop July 29, 2026 07:40
Pouyanpi added 3 commits July 29, 2026 11:16
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>
@Pouyanpi
Pouyanpi force-pushed the pouyanpi/rail-library-stack-10-http-instrumentation branch from 933c19b to b77d54f Compare July 29, 2026 09:18
@Pouyanpi
Pouyanpi marked this pull request as ready for review July 29, 2026 09:42
@Pouyanpi Pouyanpi added status: triaged Triaged by a maintainer; eligible for automated review (CodeRabbit/Greptile). and removed status: needs triage New issues that have not yet been reviewed or categorized. labels Jul 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR merge guidance

@Pouyanpi thanks for the PR. GitHub is currently blocking merge for one or more repository requirements:

  • 1 commit does not have a verified signature (b77d54f). Please sign the commits and force-push the updated branch.

Relevant guide:

@Pouyanpi Pouyanpi self-assigned this Jul 29, 2026
@Pouyanpi Pouyanpi added this to the v0.24.0 milestone Jul 29, 2026
Comment thread nemoguardrails/http/instrumented.py Outdated
@greptile-apps

greptile-apps Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Adds optional tracing and request-duration metrics around the shared HTTP client.

  • Introduces an instrumented HTTP client decorator with independently configurable tracing and metrics.
  • Records sanitized request, response, retry, error, and duration telemetry.
  • Adds shared OpenTelemetry HTTP attributes, histogram instrumentation, and instrumentation tests.

Confidence Score: 3/5

The PR is not yet safe to merge because concurrent close callers can receive a successful return even when the wrapped cleanup subsequently fails.

The retry reset fixes sequential close retries, but the boolean state conflates cleanup in progress with cleanup completed, allowing another owner to discard a client whose underlying resources remain open.

Files Needing Attention: nemoguardrails/http/instrumented.py

Important Files Changed

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
Loading
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

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds 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.

Changes

HTTP instrumentation

Layer / File(s) Summary
Telemetry contracts and instruments
nemoguardrails/tracing/constants.py, nemoguardrails/http/telemetry.py
Defines HTTP semantic attributes, duration instruments, span creation, request/response attributes, error recording, and duration measurement.
Instrumented client and public API
nemoguardrails/http/instrumented.py, nemoguardrails/http/__init__.py
Wraps HTTP clients with optional tracing and metrics, preserves close behavior, avoids duplicate instrumentation, and exposes the new APIs.
Instrumentation behavior validation
tests/http/test_instrumentation.py
Tests spans, metrics, retries, errors, propagation, passthrough behavior, idempotence, lifecycle handling, sensitive-data exclusion, and telemetry failure isolation.

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
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.26% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Results For Major Changes ⚠️ Warning This is a major feature/refactor, but the PR description lacks explicit test results or testing notes. Add a brief testing section with what was run and the outcome (or note why it couldn’t be run), plus any relevant perf/regression data.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding HTTP client instrumentation and metrics.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pouyanpi/rail-library-stack-10-http-instrumentation

Comment @coderabbitai help to get the list of available commands.

Pouyanpi and others added 2 commits July 29, 2026 12:13
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>
Comment on lines +123 to +124
if self._closed:
return

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.

P1 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.

@tgasser-nv tgasser-nv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I reviewed the first commit (sha 71ca58d) only, thanks for the second commit to show how this would integrate into IORails. Here are the comments on the first commit, can you remove the IORails-related commit before you merge?

High level comments:

  • Can you address the Greptile 3/5 and feedback?
  • Can you add tests to improve coverage?

request_duration: "Histogram"


_HTTP_DURATION_BUCKETS = [

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you add a comment saying these match the OTEL spec, with a link to that page?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs: signing size: L status: triaged Triaged by a maintainer; eligible for automated review (CodeRabbit/Greptile).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants