Add log hander attributes for retrieving the OS user and group #337
Conversation
This PR attempts to add the username and group to perf logging. This is especially good for grafana logging. This should capture the name and primary group of the user that has executed reframe. REMARKS: * capturing the username and group is not portable across OSes * The fields were updated inside the `_update_check_extras`, but they could be set during the initialization of the `LoggerAdapter`.
reframe/core/logging.py
Outdated
| @@ -343,6 +343,9 @@ def __init__(self, logger=None, check=None): | |||
| 'check_perf_ref': None, | |||
| 'check_perf_lower_thres': None, | |||
| 'check_perf_upper_thres': None, | |||
| 'check_perf_upper_thres': None, | |||
There was a problem hiding this comment.
Why is the check_perf_upper_thres two times here?
There was a problem hiding this comment.
copy and paste issues... sorry! 😯
Codecov Report
@@ Coverage Diff @@
## master #337 +/- ##
==========================================
- Coverage 91.13% 91.09% -0.04%
==========================================
Files 68 68
Lines 8231 8244 +13
==========================================
+ Hits 7501 7510 +9
- Misses 730 734 +4
Continue to review full report at Codecov.
|
reframe/core/logging.py
Outdated
| @@ -378,6 +380,24 @@ def _update_check_extras(self): | |||
| if self.check.job: | |||
| self.extra['check_jobid'] = self.check.job.jobid | |||
|
|
|||
| try: | |||
| import pwd | |||
| self.extra['username'] = pwd.getpwuid(os.geteuid()).pw_name | |||
There was a problem hiding this comment.
@vkarak is it better to get the euid or the uid?
reframe/core/logging.py
Outdated
| @@ -343,6 +343,8 @@ def __init__(self, logger=None, check=None): | |||
| 'check_perf_ref': None, | |||
| 'check_perf_lower_thres': None, | |||
| 'check_perf_upper_thres': None, | |||
| 'username': None, | |||
| 'group': None, | |||
There was a problem hiding this comment.
I suggest those being osuser and osgroup.
There was a problem hiding this comment.
Also we need to document those in the public documentation.
reframe/core/logging.py
Outdated
| try: | ||
| import getpass | ||
| self.extra['username'] = getpass.getuser() | ||
| except: |
There was a problem hiding this comment.
Bare except clauses are disallowed by PEP8. Either be more precise or catch Exception or BaseException (not this case).
reframe/core/logging.py
Outdated
| gid = pwd.getpwnam(self.extra['username']).pw_gid | ||
| self.extra['group'] = grp.getgrgid(gid).gr_name | ||
| except: | ||
| pass |
There was a problem hiding this comment.
The whole thing here is overly complicated. I suggest the following:
try:
user = getpass.getuser()
except Exception:
user = '<unknown>'
try:
group = grp.getgrgid(os.getgid()).gr_name
except KeyError:
group = '<unknown>'
self.extra['osuser'] = user
self.extra['osgroup'] = groupThe getpass() does already use pwd under the hood if necessary.
We would the extreme way proposed here only if we had actually to care about effective and real user ids. But (a) I don't think we have to and (b) if that was the case, this would have to go in utility.os_ext.
There was a problem hiding this comment.
Also @victorusu, I indeed think this should not go in the _update_check_extras. It should be in the LoggerAdapter, but the I suggest moving this query to the os_ext.utility as osuser() and osgroup() functions. These functions should return the user and group name or None, so that inside the LoggerAdapter you would do the following:
'osuser': os_ext.osuser() or '<unknown>',
'osgroup': os_ext.osgroup() or '<unknown>',There was a problem hiding this comment.
There are a couple of places in the framework that we are using os.environ['USER'] to retrieve the current user. We should replace these with the new utility function. Sorry, you asked for it! 😛
vkarak
changed the title
This PR attempts to add the username and group to perf logging.
This is especially good for grafana logging.
This should capture the name and primary group of the user that has
executed reframe.
REMARKS:
_update_check_extras, but theycould be set during the initialization of the
LoggerAdapter.