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 thread lock by loading RuntimeContext explicitly. #3763

Merged
merged 12 commits into from
May 2, 2024

Conversation

WqyJh
Copy link
Contributor

@WqyJh WqyJh commented Mar 7, 2024

Fixes #3663, #3381

The old _load_runtime_context is an function wrapper, which acquires lock and check value of _RUNTIME_CONTEXT every time the wrapped function (get_current/attach/deatch) called. Since it should be initialized after the first _load_runtime_context call, there's no need to check repeatedly.

The solution is, initialize it on module import, which should be exactly once and thread safe, and use it directly without redundant check afterwards.

@WqyJh WqyJh requested a review from a team March 7, 2024 11:48
Copy link

linux-foundation-easycla bot commented Mar 7, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@ocelotl
Copy link
Contributor

ocelotl commented Mar 7, 2024

thanks @WqyJh is this PR fixing an existing issue? If not please open one and mark this PR as fixing it ✌️

@WqyJh
Copy link
Contributor Author

WqyJh commented Mar 8, 2024

Fixes #3663 and #3381

@pmcollins
Copy link
Member

Hi @WqyJh, thanks for the change -- I'm very much in favor of this idea. Would you be able to add test coverage?

@WqyJh
Copy link
Contributor Author

WqyJh commented Mar 15, 2024

Hi @WqyJh, thanks for the change -- I'm very much in favor of this idea. Would you be able to add test coverage?

Sure, I'd like to. However, I didn't see the coverage report, therefore not sure where the uncovered parts are. AFAIK, all functions related to this modification including context.get_current(), context.attach(), context.detach() already have tests.

@pmcollins
Copy link
Member

Hi @WqyJh, thanks for the change -- I'm very much in favor of this idea. Would you be able to add test coverage?

Sure, I'd like to. However, I didn't see the coverage report, therefore not sure where the uncovered parts are. AFAIK, all functions related to this modification including context.get_current(), context.attach(), context.detach() already have tests.

That's good. What do you think about adding at least one test for _load_runtime_context as well?

@WqyJh
Copy link
Contributor Author

WqyJh commented Mar 25, 2024

Hi @WqyJh, thanks for the change -- I'm very much in favor of this idea. Would you be able to add test coverage?

Sure, I'd like to. However, I didn't see the coverage report, therefore not sure where the uncovered parts are. AFAIK, all functions related to this modification including context.get_current(), context.attach(), context.detach() already have tests.

That's good. What do you think about adding at least one test for _load_runtime_context as well?

Added an unitest test_load_runtime_context.

However, I found that OTEL_PYTHON_CONTEXT not working properly. If OTEL_PYTHON_CONTEXT=contextvars_context, an ContextVarsRuntimeContext instance would be loaded, however, if OTEL_PYTHON_CONTEXT=context, an Context instance won't be loaded. This is caused by entry_points function, the return value contains only one EntryPoint named contextvars_context.

image

It's clear that this problem is not introduced by this PR because I didn't modify these code lines. If you also think it's an abnormal behavior, I think we should merge this PR first and open a new issue for this problem.

    default_context = "contextvars_context"

    configured_context = environ.get(
        OTEL_PYTHON_CONTEXT, default_context
    )  # type: str

    try:
        return next(  # type: ignore
            iter(  # type: ignore
                entry_points(  # type: ignore
                    group="opentelemetry_context",
                    name=configured_context,
                )
            )
        ).load()()
    except Exception:  # pylint: disable=broad-except
        logger.exception(
            "Failed to load context: %s", configured_context
        )

@WqyJh
Copy link
Contributor Author

WqyJh commented Apr 2, 2024

Hi @pmcollins, this PR needs review. Would you please help review it and let's finish this PR.

@pmcollins
Copy link
Member

Changes are looking good, @WqyJh. Can you fix lint?

@WqyJh
Copy link
Contributor Author

WqyJh commented Apr 11, 2024

Changes are looking good, @WqyJh. Can you fix lint?

Yes, lint has been fixed and tested in py310, please approve the CI.

image

Copy link
Member

@pmcollins pmcollins left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Thanks for doing this.

Should this be mentioned in the changelog?

@WqyJh
Copy link
Contributor Author

WqyJh commented Apr 12, 2024

Changes LGTM. Thanks for doing this.

Should this be mentioned in the changelog?

Yes, I think it should.

@pmcollins
Copy link
Member

Please feel free to add the changelog entry, then that check will pass.

@WqyJh
Copy link
Contributor Author

WqyJh commented Apr 15, 2024

All works seems done, can it be merged now?

@lzchen
Copy link
Contributor

lzchen commented Apr 16, 2024

@WqyJh

Thanks for the change. Could you create a tracking issue for this. We will merge this pr separately.

@WqyJh
Copy link
Contributor Author

WqyJh commented Apr 17, 2024

@WqyJh

Thanks for the change. Could you create a tracking issue for this. We will merge this pr separately.

Sure, see #3857

@WqyJh WqyJh force-pushed the main branch 2 times, most recently from 69370ea to cb919f6 Compare April 18, 2024 03:24
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

opentelemetry-api/src/opentelemetry/context/__init__.py Outdated Show resolved Hide resolved
@aabmass
Copy link
Member

aabmass commented Apr 29, 2024

@WqyJh can you fix the mypy failure with the following diff?

diff --git a/opentelemetry-api/tests/context/test_context.py b/opentelemetry-api/tests/context/test_context.py                                                        
index af5b38ef8..18f6f68a5 100644                                                                                                                                     
--- a/opentelemetry-api/tests/context/test_context.py                                                                                                                 
+++ b/opentelemetry-api/tests/context/test_context.py                                                                                                                 
@@ -85,11 +85,11 @@ class TestInitContext(unittest.TestCase):                                                                                                         
         self.assertIsInstance(ctx, ContextVarsRuntimeContext)                                                                                                        
                                                                                                                                                                      
     @patch.dict("os.environ", {OTEL_PYTHON_CONTEXT: "contextvars_context"})                                                                                          
-    def test_load_runtime_context(self):                                                                                                                             
+    def test_load_runtime_context(self):  # type: ignore[misc]                                                                                                       
         ctx = context._load_runtime_context()  # pylint: disable=W0212                                                                                               
         self.assertIsInstance(ctx, ContextVarsRuntimeContext)                                                                                                        
                                                                                                                                                                      
     @patch.dict("os.environ", {OTEL_PYTHON_CONTEXT: "foo"})                                                                                                          
-    def test_load_runtime_context_fallback(self):                                                                                                                    
+    def test_load_runtime_context_fallback(self):  # type: ignore[misc]                                                                                              
         ctx = context._load_runtime_context()  # pylint: disable=W0212                                                                                               
         self.assertIsInstance(ctx, ContextVarsRuntimeContext) 

@lzchen lzchen merged commit 52abb61 into open-telemetry:main May 2, 2024
233 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants