Skip to content

Conversation

@Pouyanpi
Copy link
Collaborator

@Pouyanpi Pouyanpi commented Jul 7, 2025

Description

This PR refactors the OpenTelemetry integration to follow OpenTelemetry best practices by removing the SDK dependency and relying only on the OpenTelemetry API. We should not configure observability; that's the application's responsibility.

Changes

  • Breaking: Removed opentelemetry-sdk from [tracing] extra dependencies
  • Breaking: OpenTelemetry adapter no longer configures SDK/exporters via YAML config
  • Added deprecation warnings for old configuration parameters (exporter, resource_attributes, etc.)
  • Added validation to warn users when OpenTelemetry is not properly configured
  • Kept register_otel_exporter function with deprecation warning (will be removed in v0.16.0)

Fixes

Resolves #1094

@Pouyanpi Pouyanpi added the enhancement New feature or request label Jul 7, 2025
@Pouyanpi Pouyanpi self-assigned this Jul 7, 2025
@Pouyanpi Pouyanpi added this to the v0.15.0 milestone Jul 7, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jul 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.13%. Comparing base (ef97795) to head (873d654).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1269      +/-   ##
===========================================
+ Coverage    69.78%   70.13%   +0.34%     
===========================================
  Files          161      161              
  Lines        16057    16037      -20     
===========================================
+ Hits         11206    11248      +42     
+ Misses        4851     4789      -62     
Flag Coverage Δ
python 70.13% <100.00%> (+0.34%) ⬆️

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

Files with missing lines Coverage Δ
nemoguardrails/tracing/adapters/opentelemetry.py 93.87% <100.00%> (+89.52%) ⬆️

... and 1 file 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.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2025

Documentation preview

https://nvidia.github.io/NeMo-Guardrails/review/pr-1269

@Pouyanpi Pouyanpi force-pushed the fix/tracing-otel-api branch 3 times, most recently from 73d11fc to 6609cf2 Compare July 8, 2025 14:40
@Pouyanpi Pouyanpi changed the title feat: tracing as otel api feat!: update tracing to use otel api Jul 8, 2025
@Pouyanpi Pouyanpi requested a review from Copilot July 8, 2025 14:41
@Pouyanpi Pouyanpi changed the title feat!: update tracing to use otel api feat(tracing)!: update tracing to use otel api Jul 8, 2025
Copy link

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 refactors the OpenTelemetry integration to depend only on the API (no SDK), adds deprecation warnings for legacy config parameters, validates runtime configuration, updates tests, and refreshes documentation and examples.

  • Switched the adapter to use only opentelemetry-api and removed SDK setup from the library
  • Introduced deprecation warnings and runtime checks for old tracing configuration
  • Updated tests, examples, and documentation to reflect API-only usage

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_tracing_adapters_opentelemetry.py Adjusted tests for API-only adapter, added deprecation and warning tests
pyproject.toml Removed SDK from tracing extras; added API/SDK under docs group
nemoguardrails/tracing/adapters/opentelemetry.py Refactored adapter to use only API, added deprecation and validation
examples/configs/tracing/working_example.py New example script demonstrating proper OpenTelemetry setup
examples/configs/tracing/README.md Expanded documentation, quickstart, migration guide, and examples
Comments suppressed due to low confidence (1)

nemoguardrails/tracing/adapters/opentelemetry.py:96

  • The deprecation warning URL points to the develop branch, while other links use main. Align the branch in this link to main to avoid confusion for users browsing the default branch.
        "https://github.com/NVIDIA/NeMo-Guardrails/blob/develop/examples/configs/tracing/README.md#migration-guide",

@Pouyanpi Pouyanpi requested a review from tgasser-nv July 9, 2025 11:57
@Pouyanpi Pouyanpi marked this pull request as ready for review July 9, 2025 11:58
Copy link
Collaborator

@tgasser-nv tgasser-nv left a comment

Choose a reason for hiding this comment

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

LGTM ! Could you try out the JSON file-logging locally to check everything works before merging?

Copy link
Contributor

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

Thank you @Pouyanpi 🙌
This fixes #1094 :shipit:

Pouyanpi and others added 7 commits July 11, 2025 16:03
Refactored the OpenTelemetry tracing adapter to follow OpenTelemetry library best practices. The adapter now uses only the OpenTelemetry API, does not modify global state, and relies on the application to configure the SDK and tracer provider. Removed legacy exporter and span processor configuration from the adapter. Updated tests to reflect the new initialization logic and ensure backward compatibility with old config parameters.
@Pouyanpi Pouyanpi force-pushed the fix/tracing-otel-api branch from 573a67d to 873d654 Compare July 11, 2025 14:03
@Pouyanpi
Copy link
Collaborator Author

Note

We need to update the documentation about the changes to tracing feature

  • need docs update
  • need to move migration guide to release notes or docs

@Pouyanpi Pouyanpi merged commit 8575b88 into develop Jul 11, 2025
19 checks passed
@Pouyanpi Pouyanpi deleted the fix/tracing-otel-api branch July 11, 2025 14:10
winstonallo pushed a commit to winstonallo/NeMo-Guardrails that referenced this pull request Jul 23, 2025
* feat(tracing)!: migrate OpenTelemetry adapter to use otel API

Refactored the OpenTelemetry tracing adapter to follow OpenTelemetry library best practices. The adapter now uses only the OpenTelemetry API, does not modify global state, and relies on the application to configure the SDK and tracer provider. Removed legacy exporter and span processor configuration from the adapter. Updated tests to reflect the new initialization logic and ensure backward compatibility with old config parameters.

Signed-off-by: Pouyan <[email protected]>

Add handling for config directory with .yml/.yaml extension

Fixes `config.yml/` directory being accepted as a valid path and attempted to open as a file, resulting in a `ENOPERM`

Signed-off-by: Arthur Bied-Charreton <[email protected]>

Add test config with .yml extension

Add test verifying that general.yml/ is interpreted as a directory

Run pre-commit

Simplify condition in `from_path`

Add test config with .yml extension

Add test verifying that general.yml/ is interpreted as a directory

Simplify condition in `from_path`

Add test config with .yml extension

Add test verifying that general.yml/ is interpreted as a directory

Simplify condition in `from_path`
winstonallo pushed a commit to winstonallo/NeMo-Guardrails that referenced this pull request Jul 23, 2025
* feat(tracing)!: migrate OpenTelemetry adapter to use otel API

Refactored the OpenTelemetry tracing adapter to follow OpenTelemetry library best practices. The adapter now uses only the OpenTelemetry API, does not modify global state, and relies on the application to configure the SDK and tracer provider. Removed legacy exporter and span processor configuration from the adapter. Updated tests to reflect the new initialization logic and ensure backward compatibility with old config parameters.

Signed-off-by: Pouyan <[email protected]>

Add handling for config directory with .yml/.yaml extension

Fixes `config.yml/` directory being accepted as a valid path and attempted to open as a file, resulting in a `ENOPERM`

Signed-off-by: Arthur Bied-Charreton <[email protected]>

Add test config with .yml extension

Add test verifying that general.yml/ is interpreted as a directory

Run pre-commit

Simplify condition in `from_path`

Add test config with .yml extension

Add test verifying that general.yml/ is interpreted as a directory

Simplify condition in `from_path`

Add test config with .yml extension

Add test verifying that general.yml/ is interpreted as a directory

Simplify condition in `from_path`

Add test config with .yml extension

Add test verifying that general.yml/ is interpreted as a directory

Simplify condition in `from_path`

Add test config with .yml extension

Add test verifying that general.yml/ is interpreted as a directory

Simplify condition in `from_path`

Add test config with .yml extension

Add test verifying that general.yml/ is interpreted as a directory

Simplify condition in `from_path`
Pouyanpi added a commit that referenced this pull request Oct 1, 2025
* feat(tracing)!: migrate OpenTelemetry adapter to use otel API

Refactored the OpenTelemetry tracing adapter to follow OpenTelemetry library best practices. The adapter now uses only the OpenTelemetry API, does not modify global state, and relies on the application to configure the SDK and tracer provider. Removed legacy exporter and span processor configuration from the adapter. Updated tests to reflect the new initialization logic and ensure backward compatibility with old config parameters.

Signed-off-by: Pouyan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request status: in review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feature: remove OpenTelemetry SDK and rely only on OpenTelemetry APIs

5 participants