Re-use existing scrape sessions after scrape timeout - #25
EatonEmmerich wants to merge 15 commits into
Conversation
| if now - self.lldp_time < self.lldp_timeout: | ||
| return | ||
| await self._update_lldp() | ||
| self.lldp_time = now |
There was a problem hiding this comment.
Seems like a bug we left from the previous iteration.
EatonEmmerich
changed the title
amishatishpatel
left a comment
amishatishpatel
left a comment
There was a problem hiding this comment.
Minor comments/suggestions. Thanks for running me through the expected behaviour.
| return web.Response(text=content) | ||
|
|
||
|
|
||
| def scraper_factory(switch_factory) -> Callable[[Cache, str], Scraper]: |
There was a problem hiding this comment.
I reckon switch_factory needs a type hint itself.
| registry=registry, | ||
| ) | ||
|
|
||
| # TODO: Use a TaskGroup instead of a list of tasks to robustly handle the async context. |
There was a problem hiding this comment.
I reckon this TODO is still valuable, but I'm not sure it's possible with your current approach of Scraper.wait_for_scraper.
There was a problem hiding this comment.
So the reason this todo is still there is that this will require bumping python version from 3.6 to 3.10+
| raise TypeError(f'Expected str, got {type(result)}') | ||
| return result | ||
|
|
||
| async def maybe_refresh_port_info(self) -> None: |
There was a problem hiding this comment.
Personally, big fan of this method name, but I reckon it needs to be less mysterious/ambiguous for anyone new to this (including me).
- I reckon just calling it
refresh_port_infoshould be fine - If you really want to indicate conditions for updating, that should go in the docstring(s) of the constituent
_populate_ports()and_update_lldp()
There was a problem hiding this comment.
Reason: If I see a method called maybe_do_something() with no args in the call, I immediately think it's like a random throw of dice 😆
| if not self.done.is_set(): | ||
| await self.wait_for_scraper() | ||
| return self.registry | ||
|
|
||
| self.done.clear() | ||
| self.timeout = timeout # set timeout dynamically for scrape from params | ||
| self.exceptions.clear() | ||
| self.registry = prometheus_client.CollectorRegistry() |
There was a problem hiding this comment.
I think I'm with you here Re: "Guarding against concurrent scrape requests" (right?)
- TIL a new
asyncio.Event.is_set()returns False - I'd still prefer
self.registrybe declared before that check on Line 72
| async def maybe_refresh_port_info(self) -> None: | ||
| """Refresh the port information""" | ||
| await self._populate_ports() | ||
| await self._update_lldp() |
There was a problem hiding this comment.
Also, I know this was more a reshuffle than a feature update, but worth asking
- Do these two steps need to be run sequentially?
- Or can they happen ~simultaneously?
I was typing up a suggestion to use asyncio.gather, but that looked needlessly complicated compared to what it here. So... 🤷
There was a problem hiding this comment.
Seems like it could happen simultaneously.
2 participants
The cache now creates a "scraper" rather than a "switch"
The scraper keeps state regarding the success of the last collection, and if it hasn't been reported yet, report that instead without starting a new scrape.