Log cleanup and additional data#37
Conversation
PawelPlesniak
added
enhancement
emmuhamm
left a comment
emmuhamm
left a comment
There was a problem hiding this comment.
Thanks for this @PawelPlesniak!
I've tested this locally and I can reproduce the intended changes:
A part of me wonders if its possible to just have the (resolved) column be the default column anyway, and have an extra column for the ports, and maybe hide the 'raw' URI? I'm not sure what the use case of that is. In any case, this is just me thinking out loud and I'm happy for the current behaviour to be implemented.
There are a couple of comments I've left. Most are minor and/or clarification, and theres a bit where I saw that the logging implementation can be improved that I would much prefer to have this in :).
MSQT + regular pytests ran just fine on np04-srv-028 on jul 20 with this branch on
| if "CONNECTION_FLASK_DEBUG" in os.environ: | ||
| debug_level = int(os.environ["CONNECTION_FLASK_DEBUG"]) | ||
| else: | ||
| debug_level = 1 | ||
|
|
||
|
|
||
| def convert_log_level(log_level): | ||
| if log_level == 0: | ||
| return logging.WARNING | ||
| return 30# logging.WARNING | ||
| if log_level == 1: | ||
| return logging.INFO | ||
| return 20# logging.INFO | ||
| if log_level == 2: | ||
| return logging.DEBUG | ||
| return logging.INFO | ||
| return 10# logging.DEBUG | ||
| return 20# logging.INFO |
There was a problem hiding this comment.
This bit of code gave me despair 😩. I know that these aren't your changes, and apologies for introducing scope creep and adding a bit of work on your end, but at the very least I would suggest:
- adding an enum / dictionary / mapping to the log 'debug log level' that is obtained from
CONNECTION_FLASK_DEBUGfollowing the information in the README:
Some debug information will be printed by the connection-flask if the environment variable 'CONNECTION_FLASK_DEBUG' is set to a number greater than 0. Currently 1 will print timing information for the publish/lookup calls. 2 will give information about what was published/looked up and 3 is even more verbose printing the actual JSON of the requests.
-
immediately parsing the os.environ call of
CONNECTION_FLASK_DEBUGand parse it into alogging.[severitylevel]thing -
deleting the function
convert_log_leveland just sticking it in the log_level.
There was a problem hiding this comment.
Good suggestions, thanks. This was kept as is for the original author to see what is going on, but you're right it should be done thoroughly and correctly
| except socket.herror: | ||
| return f"No reverse DNS record found for IP: {ip_address}" | ||
| except Exception as e: | ||
| return f"Error parsing URI or resolving host: {e}" |
There was a problem hiding this comment.
Just wanted to check, do you want to return a string here or raise an error?
This may then get passed into the table, whereas the above 'invalid URI' try/except block will throw.
There was a problem hiding this comment.
Don't want to raise errors, as this would stop the service, need to log errors and continue
There was a problem hiding this comment.
Gotcha. And just for posterity, the above bit of code
if not ip_address:
raise ValueError(f"Invalid URI format: {uri}. No hostname found.")is meant to throw cuz it means the uri was ill formed?
| # current_keys = [str(k) for k in store.keys()] | ||
| # keys_str = ", ".join(str(k) for k in store.keys()) | ||
| # log.warning(f"store_keys={keys_str}") |
|
|
||
| js = json.loads(request.data) | ||
| log.debug(f"request=[{js}]") | ||
| log.warning(f"request=[{js}]") |
There was a problem hiding this comment.
Probably this?
| log.warning(f"request=[{js}]") | |
| log.debug(f"request=[{js}]") |
There was a problem hiding this comment.
Note that there are a couple of log severity changes as well, such as from info -> debug. I think the ones where I didn't comment are fine and reasonable, but just a flag up to double check if all are intended.
There was a problem hiding this comment.
Stale development, good catch
Co-authored-by: Emir Muhammad <49058518+emmuhamm@users.noreply.github.com>
Co-authored-by: Emir Muhammad <49058518+emmuhamm@users.noreply.github.com>
|
Thanks for the review. For the table rendering it may be nicer to only have the resolved URIs, as long as the underpinning dicts don't get changed (could be an issue with k8s) |
3 participants
Description
The connectivity service logfiles generated on the
developbranch are illegible, with too many entries not corresponding to useful information. The first part of this PR addresses this.The second part is to add an additional column into the
connectionstable rendered, defining a "uri (resolved)" column for this table. This is not used by the connectivity service or stored, it is just for ease of reading when the webpage is viewed.Type of change
Testing checklist
dbt-build --unittest)pytest -s minimal_system_quick_test.py)dunedaq_integtest_bundle.sh)python -m pytest)pre-commit run --all-files)Comments here on the testing
Further checks
dbt-build --lint, and/or see https://dune-daq-sw.readthedocs.io/en/latest/packages/styleguide/)(Indicate issue here: # (issue))