Skip to content

fix: respect the user-provided log options regardless of tracing configuration #1317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 14, 2025

Conversation

RobGeada
Copy link
Contributor

Description

Using tracing overrides all user-provided logging configuration. While this is necessary to generate detailed logs, it should not require returning that new log information back to the user if they did not request it. See #1316 for more detail.

Changes:

  1. Ensures that regardless of tracing configuration, the returned response object respects the requested log options.
  2. Modifies existing tracing unit test to account for new logging behavior
  3. Adds new tracing unit test to check that ll permutations of logging options are respected if tracing is configured.

Related Issue(s)

Checklist

  • I've read the CONTRIBUTING guidelines.
  • I've updated the documentation if applicable.
  • I've added tests if applicable.
  • @mentions of the person or team responsible for reviewing proposed changes.

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.60%. Comparing base (bee719b) to head (008e17e).
⚠️ Report is 11 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1317      +/-   ##
===========================================
+ Coverage    70.45%   70.60%   +0.15%     
===========================================
  Files          161      161              
  Lines        16214    16302      +88     
===========================================
+ Hits         11423    11510      +87     
- Misses        4791     4792       +1     
Flag Coverage Δ
python 70.60% <100.00%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
nemoguardrails/rails/llm/llmrails.py 90.09% <100.00%> (+0.65%) ⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Pouyanpi Pouyanpi requested review from Copilot and Pouyanpi August 7, 2025 10:33
@Pouyanpi Pouyanpi added this to the v0.16.0 milestone Aug 7, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where tracing configuration was overriding user-provided logging options in the response object. The change ensures that when tracing is enabled, comprehensive logging occurs internally for tracing purposes, but the returned response respects the user's original log configuration preferences.

  • Stores original user log options before tracing modifications
  • Filters response log data to match user's original preferences
  • Updates tests to verify new behavior across all logging option combinations

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
nemoguardrails/rails/llm/llmrails.py Adds logic to preserve original log options and filter response data accordingly
tests/test_tracing.py Updates existing tests and adds comprehensive parametrized test for all log option combinations

@Pouyanpi
Copy link
Collaborator

Pouyanpi commented Aug 7, 2025

@RobGeada Great job! Thank you very much for this PR, it looks good!
Would you please look at the open comments and resolve them.

I think this PR is in a good shape to include it for 0.16.0 release.

A minor note; please make sure all your commits are gpg signed following contributing guidelines.

@RobGeada RobGeada force-pushed the RespectLogOptions branch 2 times, most recently from cdd83ae to 5268250 Compare August 11, 2025 13:46
@RobGeada
Copy link
Contributor Author

@RobGeada Great job! Thank you very much for this PR, it looks good! Would you please look at the open comments and resolve them.

I think this PR is in a good shape to include it for 0.16.0 release.

A minor note; please make sure all your commits are gpg signed following contributing guidelines.

Hi @Pouyanpi, thanks for the review- I've addressed the comment and have rebased the branch onto a signed commit.

@Pouyanpi Pouyanpi added the bug Something isn't working label Aug 13, 2025
@Pouyanpi Pouyanpi changed the title Respect the user-provided log options regardless of tracing configuration fix: respect the user-provided log options regardless of tracing configuration Aug 13, 2025
@Pouyanpi Pouyanpi self-requested a review August 14, 2025 13:58
@Pouyanpi Pouyanpi merged commit 6da8010 into NVIDIA:develop Aug 14, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants