Skip to content

Conversation

edsiper
Copy link
Member

@edsiper edsiper commented Oct 9, 2025

Summary

  • add reference counting support to ctrace_attributes so attribute lists can be shared safely
  • expose a helper to acquire attribute references and update destroy logic to release them correctly
  • update the tail sampling processor to reuse resource and instrumentation attributes instead of deep copying them

Testing

  • cmake --build build --target ctraces-static
  • /usr/bin/cc ... -o /tmp/sampling_tail.o -c plugins/processor_sampling/sampling_tail.c

https://chatgpt.com/codex/tasks/task_e_68d46a9e0c3c8327ae947a2b45d8c40a

Summary by CodeRabbit

  • New Features
    • Added reference-counted attributes, enabling safe sharing and reuse across components.
    • Exposed an API to acquire/reuse attribute references.
  • Bug Fixes
    • Safer attribute teardown with null checks and guarded destruction to prevent premature frees.
  • Refactor
    • Sampling processor now reuses existing attribute sets instead of deep-copying, simplifying lifecycle management.
    • Reduced memory allocations and duplication for resource and instrumentation scope attributes, improving performance and stability.

@coderabbitai
Copy link

coderabbitai bot commented Oct 9, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Introduces reference counting to ctrace_attributes, adding a ref_count field and a new ctr_attributes_acquire API. The create/destroy logic now manages refcounts. Plugins update attribute handling to acquire references instead of deep-copying, removing copy helpers and adjusting resource and instrumentation scope attribute flows.

Changes

Cohort / File(s) Summary
Public API: attributes struct and acquire
lib/ctraces/include/ctraces/ctr_attributes.h
Adds unsigned int ref_count; to struct ctrace_attributes. Declares struct ctrace_attributes *ctr_attributes_acquire(struct ctrace_attributes *attr);.
Attributes refcount implementation
lib/ctraces/src/ctr_attributes.c
Initializes ref_count to 1 on create. Adds ctr_attributes_acquire to increment refcount with NULL check. Updates destroy to decrement and free only when it reaches 0; no-op on NULL.
Processor sampling: ownership shift
plugins/processor_sampling/sampling_tail.c
Replaces deep-copy helpers with ctr_attributes_acquire for resource and instrumentation_scope attributes. Removes unused copy functions and their prototypes. Adjusts creation paths to pass acquired attributes directly and update destruction paths accordingly.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant Attr as ctrace_attributes
  participant API as ctr_attributes_*()

  Caller->>API: ctr_attributes_create()
  activate API
  API->>Attr: allocate + init (ref_count=1)
  API-->>Caller: Attr*
  deactivate API

  Caller->>API: ctr_attributes_acquire(Attr*)
  activate API
  API->>Attr: ref_count++
  API-->>Caller: same Attr*
  deactivate API

  Caller->>API: ctr_attributes_destroy(Attr*)
  activate API
  API->>Attr: if (ref_count>1) ref_count-- and return
  API->>Attr: if (ref_count==1) free
  deactivate API
Loading
sequenceDiagram
  autonumber
  actor Sampler
  participant Span as Span/Context
  participant Res as Resource
  participant Scope as InstrumentationScope
  participant API as ctr_attributes_acquire

  Sampler->>Span: get resource_attrs / scope_attrs
  alt attrs present
    Sampler->>API: acquire(resource_attrs)
    API-->>Sampler: Attr*
    Sampler->>Res: set_attributes(Attr*)
    Sampler->>API: acquire(scope_attrs)
    API-->>Sampler: Attr*
    Sampler->>Scope: create(..., Attr*)
  else no attrs
    Sampler->>Res: set_attributes(NULL)
    Sampler->>Scope: create(..., NULL)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

I hop through refs, count one by one,
No more copy-burrows—less work, more fun.
Acquire, release, the carrots align,
Attributes shared, lifecycle fine.
With gentle paws I tidy the trail—
Tail-sampling sings, no double-scale.
Thump-thump: memory’s hale! 🥕🐇

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch eduardo-analyze-memory-safety-for-trace-references

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0def4e6 and 66ddc21.

📒 Files selected for processing (3)
  • lib/ctraces/include/ctraces/ctr_attributes.h (1 hunks)
  • lib/ctraces/src/ctr_attributes.c (1 hunks)
  • plugins/processor_sampling/sampling_tail.c (2 hunks)

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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant