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

Log a WARN if response time cannot be computed in access logs #13535

Closed
wants to merge 1 commit into from
Closed

Log a WARN if response time cannot be computed in access logs #13535

wants to merge 1 commit into from

Conversation

jaikiran
Copy link
Member

In context of #13129, the change in this PR adds a log message at WARN level stating that the response time cannot be computed.
Should we be logging this message just once instead of doing it per request, which can become too noisy?
/cc @gsmet

… in access logs but request start time isn't available
@ghost ghost added the area/vertx label Nov 28, 2020
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Added one comment inline.

We also need to add something like "requires quarkus.http.record-request-start-time set to true" to the lines starting by "Time taken to process the request" in the large table there: https://quarkus.io/guides/http-reference#configuring-http-access-logs .

@@ -30,6 +34,7 @@ public ResponseTimeAttribute(TimeUnit timeUnit) {
public String readAttribute(RoutingContext exchange) {
Long requestStartTime = exchange.get(VertxHttpRecorder.REQUEST_START_TIME);
if (requestStartTime == null) {
log.warn("Response time for request cannot be computed since the request start time hasn't been recorded");
Copy link
Member

Choose a reason for hiding this comment

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

I think I would rather do it in the constructor by checking the config rather than doing it per request.

And yes, I would still make it log the message once as we could probably have multiple log patterns referencing this.

Also I would make the warning explain how to fix it.

Does it make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hello @gsmet, yes that makes sense. I'm offline for a few days. I'll update this PR as soon as I'm back.

Copy link
Member

Choose a reason for hiding this comment

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

@jaikiran friendly ping :).

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I completely forgot this one. I'll refresh this PR shortly :)

Base automatically changed from master to main March 12, 2021 15:55
@jaikiran
Copy link
Member Author

Just an update. This is pending on my side. I have been trying to get this done, but these days my IntelliJ IDE hasn't been cooperating loading the whole Quarkus project and I haven't been able to find enough time to sort out that IDE issue.

@cescoffier
Copy link
Member

Any update on this one?

@jaikiran
Copy link
Member Author

Hello @cescoffier, I'll update this PR shortly.

@jaikiran
Copy link
Member Author

Closing this for now. I'm having a hard time opening the whole of Quarkus repo in my IDE these days. If no one else gets to this before me, I'll reopen a new PR.

@jaikiran jaikiran closed this Nov 26, 2021
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vertx triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants