search

grafana-toolbox / grafana-client

Python client library for accessing the Grafana HTTP API.
MIT License
106 stars 30 forks source link

feat: add async support #146

Closed Ousret closed 7 months ago

Ousret commented 10 months ago

Description

Some interest was shown on getting a PR to lead the way onto migrating from requests in the main goal to serve async. So, I took the liberty to compose that patch as proposed in panodata/grafana-client#134

Close panodata/grafana-client#134

Checklist

I am thinking on a way to test this in the CI appropriately. This effectively create some duplication, but no actual soft way to handle this task without duplication (async/sync).

codecov[bot] commented 10 months ago

Codecov Report

Attention: Patch coverage is 92.43697% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 94.19%. Comparing base (7303ec2) to head (ad1e745). Report is 15 commits behind head on main.

Files Patch % Lines
grafana_client/client.py 86.56% 9 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main panodata/grafana-client#146 +/- ## ========================================== - Coverage 94.44% 94.19% -0.26% ========================================== Files 26 26 Lines 1711 1808 +97 ========================================== + Hits 1616 1703 +87 - Misses 95 105 +10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Ousret commented 10 months ago

I have some questions, In the case of having the "mirror" code but async, do we have to recover the whole thing? Or can we, let's say, verify that the async interface isn't broken? With this, should we create a script that automate the generation of the _async submodule?

Ousret commented 7 months ago

Fixed the conflicts. I leave it to you then.

Ousret commented 7 months ago

I applied your suggestions and answered the questions.

amotl commented 7 months ago

7cdaa94 from panodata/grafana-client#162 improves CI support for your enhancements. If you like it, feel free to cherry-pick it into your branch. Otherwise, let's add it later.

Ousret commented 7 months ago

I did found a solution for the mock lib. One sec.

Ousret commented 7 months ago

Done! I looked into the code of requests_mock and did found a "clever" solution to the last issue.

Ready for merge.

amotl commented 7 months ago

Wonderful, ~300 lines less code lines touched. Merging.

Ousret commented 7 months ago

Thank you, looking forward to see it live.

amotl commented 7 months ago

We've also invited you as member to this project, so that we can better share future patches with you, giving you also chances to review them and such.

Ousret commented 7 months ago

Thank you!

amotl commented 7 months ago

looking forward to see it live

Maybe it will make sense to address this other minor issue before the release?

Ousret commented 7 months ago

Clearly yes, It should.

amotl commented 7 months ago

We thought to have successfully tested the new grafana-client on behalf of grafana-wtf already, so we released version 4.0.0, including your changes.

However, now on CI, there are errors. Maybe our validation was faulty.

Edit: Not an issue with 4.0.0, and now resolved already. Move on.

amotl commented 7 months ago

We've diverted the discussion to a different topic.

amotl commented 7 months ago

Dear Ahmed,

it looks like this operation went very well, so thank you once more for your significant contribution. There is a post to share with the community now, at Grafana Client migrated from Requests to Niquests, with added notes from our pen about how to eventually improve and move forward.

Thanks for proof-reading, if you like...?

With kind regards, Andreas.

Ousret commented 7 months ago

It looks like it went well so far, we will know better in a week or so to detect if there's any subtle consequences.

I took the time to read it, and here a few comments:

amotl commented 7 months ago

Dear Ahmed,

thanks a stack for your reply. Apologies for misspelling your name, I just fixed that typo. Thanks also for your suggestions to improve the code at grafana-wtf, I will try to follow your advise at [1] to hopefully fix [2], which is currently the most prominent downstream issue.

It looks like it went well so far, we will know better in a week or so to detect if there's any subtle consequences.

Yeah looks good so far. If we get the issues at grafana-wtf sorted out, concluding it by a corresponding release, I will be thoroughly happy.

With kind regards, Andreas.

[1] https://github.com/panodata/grafana-wtf/pull/130#discussion_r1545547301 [2] https://github.com/panodata/grafana-wtf/issues/132

amotl commented 7 months ago

grafana-wtf 0.19.0 has been released, concluding the update to Niquests, through grafana-client 4.0.0. Thank you very much, and cheers.

Announcement: https://community.panodata.org/t/grafana-wtf/67/4

Ousret commented 7 months ago

Hello again,

We are next to the one week period and everything still look okay :+1: . As far as I can see, half of the traffic uses the v4+ the other half remain on the v3.

We've applied the minor "remarks" found during our work in grafana-client at Niquests, we will soon release a new version with it.

If you have ideas, possible improvements do not hesitate to ping us at Niquests. And, lastly, if you come to know project(s) that could benefit from Niquests, feel free to introduce us :+1:

Regards,

amotl commented 7 months ago

Dear Ahmed,

thank you very much for picking up some UX improvements from here on behalf of https://github.com/jawah/niquests/pull/104. Looks good to me!

We are next to the one week period and everything still look okay 👍 . As far as I can see, half of the traffic uses the v4+ the other half remain on the v3.

Yeah, no bad reports yet, so far, and we guess it will stay like this. This means Niquests has excellent quality already, please keep up your good work.

With kind regards, Andreas.