Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove internal tag from Context::setStorage #867

Merged
merged 1 commit into from
Nov 25, 2022

Conversation

marcospassos
Copy link
Contributor

Fixes #866

@marcospassos marcospassos requested a review from a team as a code owner November 23, 2022 21:05
@welcome
Copy link

welcome bot commented Nov 23, 2022

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 23, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: marcospassos / name: Marcos Passos (418ec81)

@brettmc
Copy link
Collaborator

brettmc commented Nov 24, 2022

Hi @marcospassos I haven't worked on context in very much depth, can you elaborate on your use-case for needing custom context storage?

@marcospassos
Copy link
Contributor Author

Sure! We've been using the Swoole context for some time now, and it works perfectly. But the only way to use it is by setting it through Context::setStorage (e.g., integrating with the Swoole storage). However, this method is flagged as internal, but it doesn't make much sense since there is no other way of specifying custom contexts.

@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #867 (418ec81) into main (904da75) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #867   +/-   ##
=========================================
  Coverage     79.81%   79.81%           
  Complexity     2035     2035           
=========================================
  Files           268      268           
  Lines          5271     5271           
=========================================
  Hits           4207     4207           
  Misses         1064     1064           
Flag Coverage Δ
7.4 79.30% <ø> (ø)
8.0 79.80% <ø> (ø)
8.1 79.94% <ø> (ø)

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

Impacted Files Coverage Δ
src/Context/Context.php 82.05% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 904da75...418ec81. Read the comment docs.

@brettmc
Copy link
Collaborator

brettmc commented Nov 25, 2022

That makes sense, @marcospassos - I don't see any other way to do it. Can you please sign the CLA so that we can accept your work?
And, since you're using the library, we'd love your feedback on whether there's an easier way for others to use this module. For example, we could hook into composer's autoloader and have it set the default context storage automatically (maybe enabled by a config/env var) - I don't know whether that's viable with swoole or not, but we're trying to reduce/eliminate setup code for users.

@marcospassos
Copy link
Contributor Author

Can you please sign the CLA so that we can accept your work?

Done!

@brettmc brettmc merged commit 22c78cf into open-telemetry:main Nov 25, 2022
@marcospassos marcospassos deleted the patch-1 branch November 25, 2022 21:56
@omigafu
Copy link

omigafu commented Dec 8, 2022

Sure! We've been using the Swoole context for some time now, and it works perfectly. But the only way to use it is by setting it through Context::setStorage (e.g., integrating with the Swoole storage). However, this method is flagged as internal, but it doesn't make much sense since there is no other way of specifying custom contexts.

@marcospassos Can you provide a more detailed example? I need a concrete demo because I don't have a deep research on this, thanks!

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.

Remove internal flag from Context::setStorage
3 participants