ODSC-70796 instead of threads for each telemetry event, created a glo… #1166
Conversation
agrimk
requested review from
darenr,
mayoor,
mrDzurb,
VipulMascarenhas,
qiuosier and
ahosler
as code owners
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
oracle-contributor-agreement
bot
added
the
OCA Required
ads/telemetry/telemetry.py
Outdated
| @@ -23,7 +25,8 @@ | |||
| USER_AGENT_KEY = "additional_user_agent" | |||
| UNKNOWN = "UNKNOWN" | |||
| DELIMITER = "&" | |||
|
|
|||
| THREAD_POOL_SIZE = 16 | |||
| thread_pool = concurrent.futures.ThreadPoolExecutor(max_workers=THREAD_POOL_SIZE) | |||
There was a problem hiding this comment.
The telemetry.py and client.py modules handle different aspects of telemetry. I suggest moving the thread pool creation to client.py.
ads/aqua/extension/__init__.py
Outdated
| thread_pool.shutdown(wait=True) | ||
| web_app.log.info("Telemetry threadpool shutdown completed.") | ||
|
|
||
| web_app.io_loop.add_callback(run_on_shutdown) |
There was a problem hiding this comment.
I'm not sure about this logic. Technically user can use AQUA CLI where we capture the telemetry as well. Users don't need to run extension in this case.
There was a problem hiding this comment.
Do you mean that I should call the shutdown there as well ?
There was a problem hiding this comment.
No I men, do we really need to shut it down? In which case it would be useful?
There was a problem hiding this comment.
ok, got it. I am removing the shutdown code in that case.
I added it since it was part of the ticket ODSC-70796
ads/telemetry/client.py
Outdated
| thread.start() | ||
| return thread | ||
| thread_pool.submit(self.record_event, args=(category, action, detail), kwargs=kwargs) | ||
| return None |
There was a problem hiding this comment.
We probably don't need to return anything form this method, however it would be nice to add return typing:
def record_event_async(
self, category: str = None, action: str = None, detail: str = None, **kwargs
) -> None:
ads/aqua/extension/__init__.py
Outdated
| @@ -37,6 +37,12 @@ def load_jupyter_server_extension(nb_server_app): | |||
| [(url_path_join(route_pattern, url), handler) for url, handler in __handlers__], | |||
| ) | |||
|
|
|||
| def run_on_shutdown(): | |||
There was a problem hiding this comment.
Do we really need this? ADS can be used without the running the Jlab.
…imk/accelerated-data-science into ODSC-70796_thread_pool_for_telemetry
…o ODSC-70796_thread_pool_for_telemetry
instead of threads for each telemetry event, created a global thread pool