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

[#21090] Added the improved code for loggingResponsetime and added the link in the docs #21118

Merged
merged 8 commits into from Aug 5, 2016

Conversation

Projects
None yet
6 participants
@shiv4nsh
Copy link
Contributor

commented Aug 3, 2016

Added the improved code as per the comments on #21110 .

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Aug 3, 2016

Can one of the repo owners verify this patch?

@shiv4nsh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2016

@ktoso @johanandren : Can we merge this ? Please assign it in testing !

@Hawstein

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2016

I think the counterpart of Java DSL needs this example too. You can look into the DebuggingDirectivesExamplesTest file.

@shiv4nsh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2016

@Hawstein : I have added the javadocs and code too . Please take a look

if(response.status().isSuccess())
{
Long elapsedTime=(requestTime - System.nanoTime()) / 1000;

This comment has been minimized.

Copy link
@johanandren

johanandren Aug 4, 2016

Member

If you wanted it in microseconds, one microsecond is 1 000 000 nanoseconds

Get("/lessTime") ~> logResponseTime(complete("loggedWithGreen")) ~> check {
responseAs[String] shouldEqual "loggedWithGreen"
}
Get("/moreTime") ~> logResponseTime(requestContext => {

This comment has been minimized.

Copy link
@johanandren

johanandren Aug 4, 2016

Member

Just skip this assertion, and keep the first. (maybe change it from "loggedWithGreen" to "logged" as there is no red/green anymore)

val loggingString = s"""Logged Request:${req.method}:${req.uri}:${resp.status}:${elapsedTime}"""
LogEntry(loggingString, level)
case anythingElse =>
LogEntry(s"$anythingElse", level)

This comment has been minimized.

Copy link
@johanandren

johanandren Aug 4, 2016

Member

It isn't really anything else, but a Rejected(reasons). If it can happen depends on where it is put in the routes. So maybe capture that with the case clause just for clarity?

}
}
}

This comment has been minimized.

Copy link
@johanandren

johanandren Aug 4, 2016

Member

Indenting and linefeeds are a bit off, please follow the conventions of the other Java samples so it looks neat and good in the docs.

This comment has been minimized.

Copy link
@shiv4nsh

shiv4nsh Aug 4, 2016

Author Contributor

For the identation I use Intellij and when I try to ident it , it changes a lot of lines hence I tried to do it manually. What is off here ? Can you please point it out so that I may do it manually.

This comment has been minimized.

Copy link
@ktoso

ktoso Aug 4, 2016

Member

We follow the standard java style guide, as published by oracle http://www.oracle.com/technetwork/java/codeconvtoc-136057.html
except we use 2 spaces.

For exampel, please don't use c-style {, do put spaces around = etc.

@johanandren

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

OK TO TEST

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2016

Test FAILed.

@akka-ci akka-ci removed the validating label Aug 4, 2016

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2016

Test FAILed.

@akka-ci akka-ci removed the validating label Aug 4, 2016

@akka-ci akka-ci added validating and removed needs-attention labels Aug 4, 2016

@akka-ci akka-ci added validating tested and removed validating labels Aug 4, 2016

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2016

Test PASSed.

@shiv4nsh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2016

@Hawstein @ktoso : Can you please review it !


// handle request to optionally generate a log entry

This comment has been minimized.

Copy link
@2beaucoup

2beaucoup Aug 5, 2016

Contributor

remove empty line?

akkaResponseTimeLoggingFunction(log, requestTimestamp)(_)
}

val logResponseTime = DebuggingDirectives.logRequestResult(LoggingMagnet(printResponseTime(_)))

This comment has been minimized.

Copy link
@ktoso

ktoso Aug 5, 2016

Member

Magnet likely should not be used explicitly, just: DebuggingDirectives.logRequestResult(printResponseTime(_))

This comment has been minimized.

Copy link
@shiv4nsh

shiv4nsh Aug 5, 2016

Author Contributor

@ktoso: If I remove the LoggingMagnet here and use this
DebuggingDirectives.logRequestResult(printResponseTime(_))
: It gives me an error of TypeMismatch

This comment has been minimized.

Copy link
@ktoso

ktoso Aug 5, 2016

Member

please paste the compile error so I can actually help you.

This comment has been minimized.

Copy link
@shiv4nsh

shiv4nsh Aug 5, 2016

Author Contributor

@ktoso
It says :
Type mismatch , expected LoggingMagnet[(HttpRequest)=>(RouteResult)=>Unit], actual (LoggingAdapter)=>(HttpRequest)=>Any=>Unit

This comment has been minimized.

Copy link
@ktoso

ktoso Aug 5, 2016

Member

ok let's leave as is then hm...

@@ -30,3 +30,13 @@ Example

.. includecode2:: ../../../../code/docs/http/scaladsl/server/directives/DebuggingDirectivesExamplesSpec.scala
:snippet: logRequestResult


Example for logging response time.

This comment has been minimized.

Copy link
@ktoso

ktoso Aug 5, 2016

Member

no ., it's not a sentence

This comment has been minimized.

Copy link
@shiv4nsh

shiv4nsh Aug 5, 2016

Author Contributor

Please suggest a name , what could the heading be , can we use Building User Defined Directive ?

Example for logging response time.
----------------------------------

This example will showcase the advance logging using the DebuggingDirective.

This comment has been minimized.

Copy link
@ktoso

ktoso Aug 5, 2016

Member

The following example showcases building advanced logging directives on top of the provided DebuggingDirectives.

This comment has been minimized.

Copy link
@2beaucoup

2beaucoup Aug 5, 2016

Contributor

"advanced logging using the DebuggingDirectives."

This comment has been minimized.

Copy link
@ktoso

ktoso Aug 5, 2016

Member

Titles are not usually sentences though, no dot please ;)

----------------------------------

This example will showcase the advance logging using the DebuggingDirective.
In this example we are logging the client request and the response time taken by request to be fullfilled

This comment has been minimized.

Copy link
@ktoso

ktoso Aug 5, 2016

Member

fix spelling: fulfilled

This comment has been minimized.

Copy link
@ktoso

ktoso Aug 5, 2016

Member

add : at the end please.

This comment has been minimized.

Copy link
@ktoso

ktoso Aug 5, 2016

Member

The sentence is a bit weird though, how about:

The built `logResponseTime ` directive will log the request time (or rejection reason):

This comment has been minimized.

Copy link
@2beaucoup

2beaucoup Aug 5, 2016

Contributor

reword "taken by request"?

This comment has been minimized.

Copy link
@ktoso

ktoso Aug 5, 2016

Member

yeah

@ktoso

This comment has been minimized.

Copy link
Member

commented Aug 5, 2016

LGTM after last round of comments addressed, thanks

@akka-ci akka-ci added validating tested and removed tested labels Aug 5, 2016

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2016

Test PASSed.

@akka-ci akka-ci removed the validating label Aug 5, 2016

@akka-ci akka-ci added validating and removed tested labels Aug 5, 2016

@@ -30,3 +30,13 @@ Example

.. includecode2:: ../../../../code/docs/http/scaladsl/server/directives/DebuggingDirectivesExamplesSpec.scala
:snippet: logRequestResult


Building Advanced Directives

This comment has been minimized.

Copy link
@ktoso

ktoso Aug 5, 2016

Member

good

@ktoso

This comment has been minimized.

Copy link
Member

commented Aug 5, 2016

LGTM, thanks for your work on this.
We'll merge after validation

@ktoso ktoso added the reviewed label Aug 5, 2016

@akka-ci akka-ci added the tested label Aug 5, 2016

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2016

Test PASSed.

@akka-ci akka-ci removed the validating label Aug 5, 2016

@ktoso

This comment has been minimized.

Copy link
Member

commented Aug 5, 2016

Resolves #21090

@ktoso ktoso merged commit 25e4586 into akka:master Aug 5, 2016

2 checks passed

default Test PASSed.
Details
typesafe-cla-validator All users have signed the CLA
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.