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

feat: Add support for async instrumentation functions #61

Merged
merged 7 commits into from
Feb 24, 2023

Conversation

AndreasPB
Copy link
Contributor

No description provided.

@caspervk
Copy link

@trallnag should this be merged?

@Skeen
Copy link

Skeen commented Jun 14, 2022

Async support would be great

@trallnag
Copy link
Owner

Is this still relevant? From what I understand the new middleware implementation is async. I did not implement this, so I can't say for sure. In addition I know almost nothing about async Python.

#139

@AndreasPB
Copy link
Contributor Author

AndreasPB commented Aug 24, 2022

We use this branch at work, so it would be awesome to have it on master :)

Edit: I'll try to read through the changes to make sure

@trallnag trallnag changed the title Add async instrumentation support feat: Add support for async instrumentation functions Feb 23, 2023
@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Merging #61 (b8491ab) into master (e1b2391) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
+ Coverage   93.60%   93.75%   +0.14%     
==========================================
  Files           5        5              
  Lines         297      304       +7     
==========================================
+ Hits          278      285       +7     
  Misses         19       19              
Impacted Files Coverage Δ
...ometheus_fastapi_instrumentator/instrumentation.py 95.52% <100.00%> (+0.28%) ⬆️
...rc/prometheus_fastapi_instrumentator/middleware.py 95.23% <100.00%> (+0.17%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@trallnag
Copy link
Owner

I managed to rebase your enhancement into the new middleware. I don't have any tests yet. Can you add examples on how you use async_instrumentation? Of course only if it is still relevant for you.

I will go ahead and merge it into master regardless because the changes don't interfere in any way with existing code.

@Skeen
Copy link

Skeen commented Feb 23, 2023

... Can you add examples on how you use async_instrumentation?

Hi, we use this feature actively.

The use-case is exactly as with the non-async case:

sync_metric = Info("sync_metric", "Documentation")
async_metric = Info("async_metric", "Documentation")

def sync_function(_: InstInfo) -> None:
    value = sync_metric_getter()
    sync_metric.info({"type": value})

async def async_function(_: InstInfo) -> None:
    value = await async_metric_getter()
    async_metric.info({"type": value})

instrumentator = Instrumentator()

instrumentator.add(sync_function)
instrumentator.add(async_function)

This just adds the option of using async def functions.

@trallnag
Copy link
Owner

@Skeen, thanks for the example. Adapted it into a unit test

@Skeen
Copy link

Skeen commented Feb 24, 2023

@Skeen, thanks for the example. Adapted it into a unit test

You're awesome, thank you for the work that you do!

@trallnag trallnag merged commit b021b6c into trallnag:master Feb 24, 2023
@AndreasPB
Copy link
Contributor Author

What @Skeen said - Thank you!

One last request, would it be possible to get a release with these changes sometime in the near future, @trallnag?

@trallnag
Copy link
Owner

@AndreasPB, I'll release it already this weekend

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.

5 participants