Skip to content

Conversation

@brettmc
Copy link

@brettmc brettmc commented Apr 2, 2025

Changes

When attaching a context, store the context in the ContextGuard so that it can be retrieved.

Why?

I'm a maintainer of opentelemetry-php, and am prototyping using opentelemetry-rust to do all of the heavy lifting for a PHP API/SDK. One of the features from PHP's context API that I want to implement is being able to store a guard (we call it a scope), then later fetching it and operating on the context's active span.

Usage

In rust, what I want to do would look like this:

let guard = context.attach();
//store guard

//elsewhere
let retrieved_guard = /* get guard from storage */
let span = retrieved_guard.context().span();
//mutate and end span

I think this change works (see added test), but running the benchmarks locally, the context_attach tests have a regression which probably makes this PR a non-starter. I'm submitting just in case there's an optimization that I can't see, or the regression isn't as bad as it looks.

Benchmarks

Running locally, compared to main:

context_attach/single_cx/empty_cx                                                                            
                        time:   [26.208 ns 26.245 ns 26.284 ns]
                        change: [+49.500% +50.047% +50.608%] (p = 0.00 < 0.05)
                        Performance has regressed.

context_attach/nested_cx/empty_cx                                                                            
                        time:   [48.317 ns 48.408 ns 48.510 ns]
                        change: [+49.426% +49.904% +50.388%] (p = 0.00 < 0.05)
                        Performance has regressed.

context_attach/out_of_order_cx_drop/empty_cx                                                                            
                        time:   [37.119 ns 37.351 ns 37.644 ns]
                        change: [+26.830% +27.500% +28.154%] (p = 0.00 < 0.05)
                        Performance has regressed.

context_attach/single_cx/single_value_cx                                                                            
                        time:   [32.586 ns 32.660 ns 32.737 ns]
                        change: [+77.010% +77.527% +78.077%] (p = 0.00 < 0.05)
                        Performance has regressed.

context_attach/nested_cx/single_value_cx                                                                            
                        time:   [62.826 ns 62.945 ns 63.069 ns]
                        change: [+86.643% +87.316% +87.937%] (p = 0.00 < 0.05)
                        Performance has regressed.

context_attach/out_of_order_cx_drop/single_value_cx                                                                            
                        time:   [54.517 ns 54.585 ns 54.662 ns]
                        change: [+73.080% +73.636% +74.264%] (p = 0.00 < 0.05)
                        Performance has regressed.

context_attach/single_cx/span_cx                                                                            
                        time:   [32.646 ns 32.730 ns 32.816 ns]
                        change: [+76.620% +77.238% +77.926%] (p = 0.00 < 0.05)
                        Performance has regressed.

context_attach/nested_cx/span_cx                                                                            
                        time:   [62.843 ns 63.110 ns 63.422 ns]
                        change: [+85.580% +86.407% +87.284%] (p = 0.00 < 0.05)
                        Performance has regressed.

context_attach/out_of_order_cx_drop/span_cx                                                                            
                        time:   [52.952 ns 53.026 ns 53.098 ns]
                        change: [+72.477% +73.032% +73.663%] (p = 0.00 < 0.05)
                        Performance has regressed.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@brettmc brettmc requested a review from a team as a code owner April 2, 2025 02:05
@codecov
Copy link

codecov bot commented Apr 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.0%. Comparing base (24b92cb) to head (476b7d5).
Report is 26 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2894   +/-   ##
=====================================
  Coverage   81.0%   81.0%           
=====================================
  Files        125     125           
  Lines      23945   23968   +23     
=====================================
+ Hits       19413   19436   +23     
  Misses      4532    4532           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

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

If we decide to add this type of API, I have another solution in mind that doesn't add any overhead.

@scottgerring
Copy link
Member

@brettmc you proposed an alternate solution that carries the context & guard together on the PHP side - what do you see the downside of this being?

I think in general we should minimize additions to our public API surface, especially where they seem 'narrow' - in a sense that we don't expect them to be broadly used.

@brettmc
Copy link
Author

brettmc commented Apr 3, 2025

you proposed an alternate solution that carries the context & guard together on the PHP side - what do you see the downside of this being?

Not terrible downsides, more complicated implementation for me but that is all, it will still work and do what I want.
If it's not likely to be useful for other folks, then I completely understand wanting to keep the public API small.

@cijothomas
Copy link
Member

Based on the SIG discussion, given there is an alternative available, we should not expose additional APIs - Closing.

@brettmc Let us know if we need to revisit this.

@cijothomas cijothomas closed this Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants