Skip to content

Remove internal tag from Context::setStorage#867

Merged
brettmc merged 1 commit into
open-telemetry:mainfrom
marcospassos:patch-1
Nov 25, 2022
Merged

Remove internal tag from Context::setStorage#867
brettmc merged 1 commit into
open-telemetry:mainfrom
marcospassos:patch-1

Conversation

@marcospassos

Copy link
Copy Markdown
Contributor

Fixes #866

@marcospassos
marcospassos requested a review from a team November 23, 2022 21:05
@welcome

welcome Bot commented Nov 23, 2022

Copy link
Copy Markdown

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

linux-foundation-easycla Bot commented Nov 23, 2022

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

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

@brettmc

brettmc commented Nov 24, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

codecov Bot commented Nov 25, 2022

Copy link
Copy Markdown

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

brettmc commented Nov 25, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

omigafu commented Dec 8, 2022

Copy link
Copy Markdown

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