Skip to content

Fix @metric_scope for generator and async generator functions #113

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

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

acradu
Copy link
Contributor

@acradu acradu commented Mar 14, 2024

Issue #, if available:

Description of changes:
This change updates @metric_scope to iterate through the wrapped generator, allowing metric collection on each iteration

@acradu acradu marked this pull request as ready for review March 14, 2024 21:58
@acradu acradu marked this pull request as draft March 14, 2024 22:02
@acradu acradu marked this pull request as ready for review March 14, 2024 22:04
@fernandoaguilar
Copy link

anyone plan on merging this in? this fixes the no current running event loop error for us, we will have to fork the repo otherwise if this is not merged in. @acradu

Copy link

@johnjang johnjang left a comment

Choose a reason for hiding this comment

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

LGTM

@johnjang johnjang merged commit 1836dd8 into awslabs:master Oct 2, 2024
1 check passed
@drduhe
Copy link

drduhe commented Oct 18, 2024

Can you do a latest release with this change?

@julius-yang
Copy link

Can you do a latest release with this change?

Seconded, we are currently using a fork with this change but it would be nice to point to this repo again.

@hussam789
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix async generator exception handling

The exception handling for async generators is incorrect. StopAsyncIteration
should be caught instead of StopIteration for async generators. The current code
will not properly handle the end of the async generator.

aws_embedded_metrics/metric_scope/init.py [28-37]

 try:
     fn_gen = fn(*args, **kwargs)
     while True:
         result = await fn_gen.__anext__()
         await logger.flush()
         yield result
 except Exception as ex:
     await logger.flush()
-    if not isinstance(ex, StopIteration):
+    if not isinstance(ex, StopAsyncIteration):
         raise
  • Apply this suggestion
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the async generator’s termination should be signaled by StopAsyncIteration rather than StopIteration, which is a subtle but important bug fix for async generators.

Medium
Fix event loop handling

Using asyncio.run() in the synchronous wrapper can cause issues if called from a
context where an event loop is already running. This can lead to "RuntimeError:
This event loop is already running". Consider using a more compatible approach
for running the async flush operation.

aws_embedded_metrics/metric_scope/init.py [84-85]

 finally:
-    asyncio.run(logger.flush())
+    try:
+        loop = asyncio.get_event_loop()
+        if loop.is_running():
+            # Create a future to run in the existing loop
+            future = asyncio.run_coroutine_threadsafe(logger.flush(), loop)
+            future.result()
+        else:
+            loop.run_until_complete(logger.flush())
+    except RuntimeError:
+        # No event loop in this thread, create a new one
+        loop = asyncio.new_event_loop()
+        loop.run_until_complete(logger.flush())
+        loop.close()
  • Apply this suggestion
Suggestion importance[1-10]: 6

__

Why: The suggestion addresses a potential RuntimeError when asyncio.run() is invoked in an environment with an already running event loop, but the proposed solution may add unnecessary complexity in contexts where it might not be needed.

Low
Improve event loop handling

Using asyncio.run() inside a generator function can cause issues as it creates
and tears down a new event loop on each iteration. This is inefficient and can
lead to problems if there are other async operations in progress. Consider using
a more efficient approach for flushing the logger.

aws_embedded_metrics/metric_scope/init.py [48-57]

 try:
     fn_gen = fn(*args, **kwargs)
+    loop = asyncio.new_event_loop()
     while True:
         result = next(fn_gen)
-        asyncio.run(logger.flush())
+        loop.run_until_complete(logger.flush())
         yield result
 except Exception as ex:
-    asyncio.run(logger.flush())
+    loop.run_until_complete(logger.flush())
+    loop.close()
     if not isinstance(ex, StopIteration):
         raise
  • Apply this suggestion
Suggestion importance[1-10]: 5

__

Why: While the suggestion calls out potential inefficiencies in repeatedly calling asyncio.run(), the approach of creating a new event loop may introduce unforeseen complexity and is not clearly superior to the existing implementation.

Low
  • More

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.

8 participants