Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Low Prio] add debug_timer decorator and other logs to auth.py #356

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

kdelee
Copy link
Member

@kdelee kdelee commented Apr 26, 2024

@john-westcott-iv lmk if this is harmless enough to include

Copy link

sonarcloud bot commented Apr 26, 2024

@@ -56,6 +70,8 @@ def parse_jwt_token(self, request):
"is_superuser": validated_body["is_superuser"],
},
)
delta_ms = (time.time() - start) * 1000
logger.debug(f'Getting/saving user took {delta_ms:.0f} (ms)')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to refactor this code into a method and them apply the decorator the same as elsewhere.

@AlanCoding
Copy link
Member

My quick 2 cents, following the DAB philosophy.

The promise of DAB is to standardize things. Having a one-off utility method like debug_timer is fine in other libraries, and is fine if there's a valid case that the utility will not be encountered again soon. But here is a method in AWX which does what that does with bells & whistles:

https://github.com/ansible/awx/blob/4754819a0916cbd9352550fb0f8fbc65273854b0/awx/main/utils/common.py#L1197-L1225

So I want to make the case here that we already have a widely shared need established.

That established, the (1)st change I'd seek is putting this into proper ansible_base/lib. But I would go further, the (2)nd change I'd like to see is setting us up to delete log_excess_runtime in AWX.

I don't love everything about that method. I'd suggest that long-term it would be better to use a local logger (starting with ansible_base.lib) as opposed to passing in a logger in my prior implementation. This would allow us to have special policies for performance logs from ansible_base in the logger configuration. Anything else you want to change about its feature set, please have a shot at it.

@john-westcott-iv
Copy link
Member

I agree with Alan on this. Personally I can't decide if it should be ansible_base.lib.decerators or ansible_base.lib.performance. I kind of like the idea of performace because then you would end up with a logger like ansible_base.lib.performance and anything else you put in there would use that same logger. @kdelee if you don't have the time to dedicate to this now please cut a JIRA and we can do this as part of 2.5-next.

@john-westcott-iv john-westcott-iv added the comments left This PR was reviewed with comments label May 2, 2024
@john-westcott-iv
Copy link
Member

@kdelee do you have capacity to make the requested changes?

@john-westcott-iv john-westcott-iv changed the title add debug_timer decorator and other logs to auth.py [Low Prio] add debug_timer decorator and other logs to auth.py May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comments left This PR was reviewed with comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants