opentelemetry-cpp icon indicating copy to clipboard operation
opentelemetry-cpp copied to clipboard

SDK Implement collection limits

Open marcalff opened this issue 8 months ago • 7 comments
trafficstars

Implement the various limits defined in the specifications:

  • events collection size limit
  • attribute collection size limit
  • links collection size limit
  • https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#span-limits
  • https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/README.md#attribute-limits

marcalff avatar Mar 20 '25 21:03 marcalff

This issue is available for anyone to work on. Make sure to reference this issue in your pull request. :sparkles: Thank you for your contribution! :sparkles:

github-actions[bot] avatar Mar 20 '25 21:03 github-actions[bot]

Can I work on this?

XueSongTap avatar Mar 21 '25 06:03 XueSongTap

I don't understand what this means....

On Fri, Mar 21, 2025, 1:40 AM Xiaochuan Ye @.***> wrote:

Can I work on this?

— Reply to this email directly, view it on GitHub https://github.com/open-telemetry/opentelemetry-cpp/issues/3303#issuecomment-2742481714, or unsubscribe https://github.com/notifications/unsubscribe-auth/BNKC6HEZDN7XOUOADSSCRSL2VOX53AVCNFSM6AAAAABZOQNV7OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONBSGQ4DCNZRGQ . You are receiving this because you are subscribed to this thread.Message ID: @.***> [image: XueSongTap]XueSongTap left a comment (open-telemetry/opentelemetry-cpp#3303) https://github.com/open-telemetry/opentelemetry-cpp/issues/3303#issuecomment-2742481714

Can I work on this?

— Reply to this email directly, view it on GitHub https://github.com/open-telemetry/opentelemetry-cpp/issues/3303#issuecomment-2742481714, or unsubscribe https://github.com/notifications/unsubscribe-auth/BNKC6HEZDN7XOUOADSSCRSL2VOX53AVCNFSM6AAAAABZOQNV7OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONBSGQ4DCNZRGQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

TheMenisFactor avatar Mar 21 '25 06:03 TheMenisFactor

Implementation Proposal for Collection Limits in OpenTelemetry C++ SDK

Overview

To implement collection limits as specified in Issue #3303, I propose the following changes to the OpenTelemetry C++ SDK:

  1. Create a new SpanLimits class to encapsulate all limit configurations
  2. Extend the TracerConfig class to include span limits
  3. Modify the Span implementation to track and enforce these limits

New Components

SpanLimits Class

Create a new class in sdk/include/opentelemetry/sdk/trace/span_limits.h with:

  • Constants for default limit values:

    • kDefaultAttributeCountLimit = 128
    • kDefaultEventCountLimit = 128
    • kDefaultLinkCountLimit = 128
    • kDefaultAttributePerEventCountLimit = 128
    • kDefaultAttributePerLinkCountLimit = 128
  • Getter/setter methods for each limit

  • Factory method for default configuration

  • Equality operator for comparison

Modifications to Existing Components

TracerConfig Class

Extend sdk/include/opentelemetry/sdk/trace/tracer_config.h to:

  • Add a SpanLimits member variable
  • Add getter/setter methods for span limits
  • Update constructor to accept span limits
  • Add a Create() factory method for custom configurations

Span Class

Modify sdk/src/trace/span.h and span.cc to:

  • Add counters for tracking collection sizes:

    • attribute_count_
    • event_count_
    • link_count_
  • Add flags to track warning status:

    • attribute_limit_warned_
    • event_limit_warned_
    • link_limit_warned_
  • Add helper methods for limit checking and logging

Implementation Approach

  1. In SetAttribute():

    • Check if attribute_count_ < GetSpanLimits().GetAttributeCountLimit()
    • If below limit: add attribute and increment counter
    • If at limit: discard and log warning (only once)
  2. In AddEvent() methods:

    • Check event count against event limit
    • For events with attributes, check attribute count against per-event limit
  3. In AddLink() methods:

    • Check link count against link limit
    • For links with attributes, check attribute count against per-link limit

Configuration

  • All limits will have reasonable defaults based on the specification
  • Limits will be configurable through the TracerConfig
  • The implementation will be backward compatible with existing code

This approach ensures we meet the requirements in the specification while maintaining flexibility for users who need custom limit configurations.

XueSongTap avatar Mar 23 '25 05:03 XueSongTap

Thanks for the analysis.

The otel-cpp SDK does not copy or buffer attributes internally; instead, they are directly serialized from user memory by the exporter. As a result, it doesn't contribute to memory growth — one of the key concerns the spec addresses with recommended attribute limits.

Introducing limit checks in the SDK would require additional memory writes and branching on the hot path, which could impact performance — especially in high-throughput or non-batching concurrent/real-time processors where efficiency is critical.

As I understand the specs, while the spec encourages (SHOULD) applying limits, it doesn't make it mandatory and only requires them to be configurable if they are implemented. In the case of otel-cpp, it seems more appropriate for specific exporters to handle such enforcement if needed. Implementing limits in the SDK would impact all exporters and users, even those who do not face issues related to excessive attributes — introducing overhead where it's not needed.

lalitb avatar Mar 23 '25 07:03 lalitb

@lalitb Thank you for your review and valuable feedback. I fully understand and share your concerns regarding performance, especially considering the direct serialization architecture of the C++ SDK. Based on your suggestions, i'm planning to modify our proposal with the following key changes:

Primary Modifications

  1. Limits Disabled by Default:

    • All limit checks will be disabled by default (using kNoLimit = UINT32_MAX value)
    • This ensures existing users experience no performance impact from this feature
  2. Explicit Opt-in Mechanism:

    • Limit functionality will only be active when users explicitly enable it through configuration
    • We'll provide a clean API for users who need this feature to easily enable it
  3. Optimized Code Path:

    • Implement a "fast path" design that requires only a single simple check in the default case (limits disabled)
    • Avoid additional branching or memory operations in the hot path
  4. Flexible Configuration Options:

    • Allow users to adopt specification-recommended values or define custom limit values
    • Support selectively enabling specific limits (e.g., only limit event count without limiting attributes)

Implementation Approach

// Fast path example
void Span::SetAttribute(nostd::string_view key, const common::AttributeValue &value) noexcept {
  // Existing code...
  
  // First check if limits are enabled (default is disabled)
  const auto &limits = GetSpanLimits();
  uint32_t attribute_limit = limits.GetAttributeCountLimit();
  
  // Fast path - most users will use this path (no limits)
  if (attribute_limit == SpanLimits::kNoLimit) {
    recordable_->SetAttribute(key, value);
    return;  // Return directly, no additional operations
  }
  
  // Only users who explicitly enable limits will execute the following code
  // Limit check logic...
}

I completely agree that performance should be the primary consideration for the C++ SDK. This revised design ensures:

  1. Compliance with the specification's SHOULD requirement (providing limits functionality without mandating its use)
  2. Virtually no performance impact for users who choose not to use limits
  3. A solution for users who need protection against excessive resource usage

This balanced approach should satisfy the needs of all users while maintaining the high-performance characteristics of the C++ SDK. Leave comment if you have any questions or concerns.

XueSongTap avatar Mar 23 '25 11:03 XueSongTap

@XueSongTap i would be still reluctant to add the support within the SDK, specifically adding any new member variables to span class, and increase the span object size.

lalitb avatar Mar 25 '25 04:03 lalitb