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

ajk-service-request-timer #1939

Merged
merged 3 commits into from
Apr 12, 2018
Merged

ajk-service-request-timer #1939

merged 3 commits into from
Apr 12, 2018

Conversation

andreak
Copy link
Member

@andreak andreak commented Feb 4, 2018

Refactor timing of servicing a request out to ServiceRequestTimer.
The default-implementation, StandardServiceTimer, gives the same
logging as the old parameter logServiceRequestTiming=true

@andreak
Copy link
Member Author

andreak commented Feb 7, 2018

Note that the codacy-report complaining about

Usage of get on optional type.
LiftRules.serviceRequestTimer.get.vend.logTime(req, resp)(doService)

https://app.codacy.com/app/Lift/framework/file/14407628970/issues/source?bid=2087836&fileBranchId=6459480#l162

is wrong, as this isn't an Option but a ValueHolder.

@@ -132,6 +132,9 @@ trait HTTPProvider {
}

private def postBoot {
if (!LiftRules.logServiceRequestTiming) {
LiftRules.installServiceRequestTimer(NoOpServiceTimer)
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I quite understand this line. If request timing is off, we install the no-op timer in the new place?

Copy link
Member Author

Choose a reason for hiding this comment

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

What I'm proposing is to remove logServiceRequestTiming and only use ServiceRequestTimer, but we cannot make such a breaking change (removinglogServiceRequestTiming) in 3.x can we? So to not break existing apps the line in postBoot installs the NoOpServiceTimer implementation which will give the same behavior as today's logServiceRequestTiming=false. Currently logServiceRequestTiming defaults to true, which corresponds to having StandardServiceTimer as default, which is the default in this PR.

Copy link
Member

@farmdawgnation farmdawgnation left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I'll probably give it a test run before merging but I think this is good for 3.3.0-M2!

@farmdawgnation farmdawgnation merged commit a08f7e9 into master Apr 12, 2018
@farmdawgnation farmdawgnation deleted the ajk-service-request-timer branch April 12, 2018 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants