Superskyyy commented on code in PR #231: URL: https://github.com/apache/skywalking-python/pull/231#discussion_r952856194
########## skywalking/agent/__init__.py: ########## @@ -37,7 +38,7 @@ __started = False __protocol = Protocol() # type: Protocol __heartbeat_thread = __report_thread = __log_report_thread = __query_profile_thread = __command_dispatch_thread \ - = __send_profile_thread = __queue = __log_queue = __snapshot_queue = __finished = None + = __send_profile_thread = meter_service_thread = __queue = __log_queue = __snapshot_queue = __finished = None Review Comment: @jiang1997 Thanks, good finding! I believe `__` prefixed variables are not supposed to be referred to by external classes, but when it is defined outside of classes, it doesn't do much other than the meaning of "you probably shouldn't use (import) this". Anyway, I see the current implementation bit out of sync with the previous reporter styles. That is, you provided a new `MeterService(Thread)`, and the result of defining threads outside of `agent/__init__.py` is that the code base becomes weird in terms of calling topology, and there are some odd references because of it. Maybe try two steps to make our agent startup behaviour better organized, given `__init__.py` was never intended to be this big and also not supposed to have logic scattered around in other modules. 1. Follow the existing style to see how you can write the MeterService class without the Threading part, like the existing profiling service (put the threading part back to `agent/__init__.py`. (If you understand this already, look at step 2) 2. Try to make the existing `__init__.py` into more organized modules at the `agent/` directory; essentially, minimize `agent/__init__.py` and decouple the current mess. (If you meet obstacles, revert to step 1, and it will be good enough) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
