openllmetry icon indicating copy to clipboard operation
openllmetry copied to clipboard

feat(traceloop-sdk): add sampling_rate parameter to control trace volume

Open Nikhil172913832 opened this issue 2 months ago • 12 comments

Add a simplified sampling_rate parameter to Traceloop.init() for easy trace sampling configuration without manually creating sampler objects.

Closes #2109

  • [x] I have added tests that cover my changes.
  • [ ] If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • [x] PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • [ ] (If applicable) I have updated the documentation accordingly.

[!IMPORTANT] Add sampling_rate parameter to Traceloop.init() for trace sampling control, with validation and conflict checks, and corresponding tests.

  • New Features:
    • Added sampling_rate parameter to Traceloop.init() in __init__.py to control trace volume (0.0–1.0).
    • Raises error if both sampling_rate and sampler are provided.
    • Validates sampling_rate is between 0.0 and 1.0.
  • Tests:
    • Added test_sampling_rate.py to test initialization with sampling_rate, invalid values, conflict with sampler, and edge cases (0.0 and 1.0).

This description was created by Ellipsis for fdb5d632750c35a01e2d18bac9a6f6b150452597. You can customize this summary. It will automatically update as commits are pushed.


Summary by CodeRabbit

  • New Features
    • Added a sampling-rate option to SDK initialization to control trace volume (0.0–1.0). Supplying a sampling rate will configure an internal ratio-based sampler; providing both a sampling rate and a custom sampler is disallowed and raises an error.
  • Tests
    • Added tests covering initialization with sampling rate, invalid values, mutual-exclusivity with a custom sampler, and edge cases (0.0 and 1.0).

Nikhil172913832 avatar Oct 11 '25 13:10 Nikhil172913832

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Oct 11 '25 13:10 CLAassistant

[!NOTE]

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds a sampling_rate parameter to Traceloop.init that validates bounds [0.0–1.0], enforces mutual exclusivity with an explicit sampler, and constructs a TraceIdRatioBased sampler when provided. Adds tests verifying init behavior, validation errors, sampler conflict, and edge cases.

Changes

Cohort / File(s) Summary
Sampling rate feature (SDK)
packages/traceloop-sdk/traceloop/sdk/__init__.py
Added optional sampling_rate parameter to init. Validate sampling_rate ∈ [0.0,1.0]. Raise ValueError if both sampler and sampling_rate provided. Construct TraceIdRatioBased sampler from sampling_rate.
Tests for sampling rate
packages/traceloop-sdk/tests/test_sampling_rate.py
New test module TestSamplingRate with pytest fixture clean_tracer_wrapper() to manage TracerWrapper.instance. Tests: valid init with sampling_rate, invalid values, conflict when both sampler and sampling_rate given, and edge cases 0.0 and 1.0. Uses OpenTelemetry test utilities.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor App as Application
    participant Init as Traceloop.init
    participant Validate as Param Validation
    participant Sampler as TraceIdRatioBased Factory
    participant TW as TracerWrapper

    App->>Init: init(..., sampler=?, sampling_rate=?)
    Init->>Validate: check mutual exclusivity & bounds
    alt both provided
        Validate-->>App: ValueError (sampler and sampling_rate)
    else out of range
        Validate-->>App: ValueError (sampling_rate out of [0.0,1.0])
    else sampling_rate provided
        Init->>Sampler: TraceIdRatioBased(sampling_rate)
        Sampler-->>Init: sampler
        Init->>TW: construct TracerWrapper with sampler
        TW-->>App: return client or None
    else no sampling_rate
        Init->>TW: construct TracerWrapper (existing/default sampler)
        TW-->>App: return client or None
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A hop, a skip, a sampled trace,
I sniff the bytes at measured pace.
Half the carrots, still the chase,
Bounds kept tidy, no double space.
I thump with joy — the traces play. 🥕🐇

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the main change—adding a sampling_rate parameter to the traceloop-sdk API to control trace volume—and aligns directly with the core objective of the pull request without extraneous detail.
Linked Issues Check ✅ Passed The implementation adds the sampling_rate parameter with proper range validation and mutual‐exclusivity checks, constructs a TraceIdRatioBased sampler when requested, and delivers comprehensive tests covering initialization, validation, conflict handling, and edge cases, fully satisfying the requirements of issue #2109.
Out of Scope Changes Check ✅ Passed All changes are exclusively related to introducing and testing the sampling_rate feature as specified in the linked issue, with no unrelated code modifications present.
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

coderabbitai[bot] avatar Oct 11 '25 13:10 coderabbitai[bot]

Hi @nirga, just wanted to check if you could take a look at this PR when you get a chance.

Nikhil172913832 avatar Oct 24 '25 08:10 Nikhil172913832

Thanks for the review! I’ll update the PR with the suggested changes

Nikhil172913832 avatar Oct 29 '25 13:10 Nikhil172913832

@nirga I might be missing something, but from my review it seems the sampling_rate does reach the TracerProvider. Here’s the flow I traced: User initializes with Traceloop.init(app_name="my_app", sampling_rate=0.5) At line 150 in packages/traceloop-sdk/traceloop/sdk/init.py, a TraceIdRatioBased sampler is created At line 161, that sampler is passed to the TraceWrapper At line 90 in packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py, it’s passed to init_tracer_provider Finally, at line 417 in the same file, the sampler is passed to the TracerProvider

Nikhil172913832 avatar Oct 29 '25 16:10 Nikhil172913832

@Nikhil172913832 I think you forgot to push packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py

nirga avatar Oct 30 '25 14:10 nirga

@nirga The lines I am referring to in packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py already exist in the main branch — I haven’t made any edits to them in this PR.

Nikhil172913832 avatar Oct 30 '25 14:10 Nikhil172913832

Regarding the tests, I took inspiration from test_sample_initialization to improve coverage and structure. Let me know if there’s anything else I can refine or expand.

"""Tests for sampling_rate parameter functionality."""

import pytest
from opentelemetry.sdk.trace.sampling import TraceIdRatioBased
from opentelemetry.sdk.trace.export.in_memory_span_exporter import InMemorySpanExporter
from opentelemetry import trace
from traceloop.sdk import Traceloop
from traceloop.sdk.tracing.tracing import TracerWrapper


def test_sampling_rate_validation():
    """Test that sampling_rate validates input range."""
    exporter = InMemorySpanExporter()

    with pytest.raises(ValueError, match="sampling_rate must be between 0.0 and 1.0"):
        Traceloop.init(
            app_name="test-invalid-high",
            sampling_rate=1.5,
            exporter=exporter,
            disable_batch=True
        )

    with pytest.raises(ValueError, match="sampling_rate must be between 0.0 and 1.0"):
        Traceloop.init(
            app_name="test-invalid-low",
            sampling_rate=-0.1,
            exporter=exporter,
            disable_batch=True
        )


def test_sampling_rate_and_sampler_conflict():
    """Test that providing both sampling_rate and sampler raises ValueError."""
    exporter = InMemorySpanExporter()
    sampler = TraceIdRatioBased(0.5)

    with pytest.raises(ValueError, match="Cannot specify both 'sampler' and 'sampling_rate'"):
        Traceloop.init(
            app_name="test-conflict",
            sampling_rate=0.5,
            sampler=sampler,
            exporter=exporter,
            disable_batch=True
        )


def test_sampling_rate_creates_spans():
    """Test that sampling_rate parameter works correctly."""
    if hasattr(TracerWrapper, "instance"):
        _trace_wrapper_instance = TracerWrapper.instance
        del TracerWrapper.instance

    try:
        exporter = InMemorySpanExporter()
        Traceloop.init(
            app_name="test-sampling",
            sampling_rate=1.0,
            exporter=exporter,
            disable_batch=True
        )

        tracer = trace.get_tracer(__name__)
        for i in range(5):
            with tracer.start_as_current_span(f"test_span_{i}"):
                pass

        spans = exporter.get_finished_spans()
        test_spans = [s for s in spans if s.name.startswith("test_span_")]
        assert len(test_spans) == 5

    finally:
        exporter.clear()
        if '_trace_wrapper_instance' in locals():
            TracerWrapper.instance = _trace_wrapper_instance
'''

Nikhil172913832 avatar Oct 30 '25 15:10 Nikhil172913832

@Nikhil172913832 they're not here in the PR

nirga avatar Oct 30 '25 15:10 nirga

@nirga I have not yet committed the test. I wanted to have your opinion on their coverage and quality. I have tested them locally and they do pass.

Nikhil172913832 avatar Oct 30 '25 15:10 Nikhil172913832

@Nikhil172913832 i meant that the code itself is not complete. The tracing.py file is not here

nirga avatar Oct 30 '25 16:10 nirga

@nirga , I haven’t modified packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py — the lines I referred to already exist in the main branch. My PR only adds the sampling_rate parameter and related logic in init.py along with the new tests. I mentioned tracing.py just to show where the sampler is passed through, not as part of the changes here.

Nikhil172913832 avatar Oct 30 '25 16:10 Nikhil172913832

Hi @nirga, I have updated the tests and cross verified that the sampling rate is being passed to the tracer provider as per your request. Let me know if any other changes are required.

Nikhil172913832 avatar Dec 12 '25 09:12 Nikhil172913832