Re: [PR] Re-adding support for fractional seconds in access log [tomcat]
jose-galvez commented on PR #720: URL: https://github.com/apache/tomcat/pull/720#issuecomment-2066644940 I like it!! Definitely way better than my change which now seems like a bad hack 藍 I can close this PR in favor of yours -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Re-adding support for fractional seconds in access log [tomcat]
jose-galvez closed pull request #720: Re-adding support for fractional seconds in access log URL: https://github.com/apache/tomcat/pull/720 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Re-adding support for fractional seconds in access log [tomcat]
ChristopherSchultz commented on PR #720: URL: https://github.com/apache/tomcat/pull/720#issuecomment-2066468644 Please have a look at a different PR which builds on some refactoring I just pushed: https://github.com/apache/tomcat/pull/721 I think I like mine better. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Re-adding support for fractional seconds in access log [tomcat]
jose-galvez commented on PR #720: URL: https://github.com/apache/tomcat/pull/720#issuecomment-2064310044 > Are you specifically talking about the ExtendedAccessLogValve's `time-taken` token? If so, you're right, that documentation did not change yet the underlying implementation did change. Yes, sorry I didn't state this better but the change I made was specifically for the `time-taken` token to go back to the Tomcat 9 behavior and report the seconds with fractions as the documentation states > IMHO, "fractional seconds" isn't worth the computation power to format it in the first place, then parse it later back into milliseconds or whatever. I would recommend changing your tooling to look for an embedded `.` character and changing behavior to be backward- and forward-compatible by simply using milliseconds or microseconds, whichever you prefer. I'll talk to our team to see if they can make these changes. For now we created a patch for our application with this PR's change so we load the modified `ExtendedAccessLogValve` class in our application for Tomcat 10. Of course this is meant to be temporary since it could become a bit of a nightmare to maintain long term > We should update the documentation for `ExtendedAccessLogValve` to make it clear that `time-taken` has changed along with `%T` to be whole-seconds. This would be great, that way we can also send a notice to our customers pointing to it so they can update any tooling they have that relies on the current format > We can continue this conversation to determine what _else_ we might do, such as continuing to support "fractional seconds" as an output format. The problem (for you) with supporting fractional-seconds is that it won't work from 10.1.0 - 10.1.23 at least, so you will have a specific set of Tomcat versions that are supported by your product and tooling. We are fine right now because our current production versions are running Tomcat 9.x and we have the patch for 10.1.x ready for when it goes to production so if there's a new output in the near future to have the fractional seconds we could just move to that and remove our internal patch but like I said I'll bring it up with the team to see if we can start moving to using milliseconds instead since it probably would also make calculations faster on our scripts and monitoring systems. Thank you so much for your comments and your insight on this issue -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Re-adding support for fractional seconds in access log [tomcat]
ChristopherSchultz commented on PR #720:
URL: https://github.com/apache/tomcat/pull/720#issuecomment-2063868914
Oh, I'm' sorry I didn't pick-up on the subtleties of your report.
Tomcat 10.1 is documented that the value for `%T` is in (presumably _whole_)
seconds. Tomcat 9 says it's in fractional seconds and that in Tomcat 10 it will
be truncated to whole-seconds. Are you specifically talking about the
ExtendedAccessLogValve's `time-taken` token? If so, you're right, that
documentation did not change yet the underlying implementation did change.
I can see some scope for still supporting all of these. For example, `%T`
can be `%{us}T` to get microseconds (now a synonym for `%D` so why not
introduce a new flavor e.g. `%{fs}T` for "fractional seconds".
IMHO, "fractional seconds" isn't worth the computation power to format it in
the first place, then parse it later back into milliseconds or whatever. I
would recommend changing your tooling to look for an embedded `.` character and
changing behavior to be backward- and forward-compatible by simply using
milliseconds or microseconds, whichever you prefer.
We should update the documentation for `ExtendedAccessLogValve` to make it
clear that `time-taken` has changed along with `%T` to be whole-seconds.
We can continue this conversation to determine what _else_ we might do, such
as continuing to support "fractional seconds" as an output format. The problem
(for you) with supporting fractional-seconds is that it won't work from 10.1.0
- 10.1.23 at least, so you will have a specific set of Tomcat versions that are
supported by your product and tooling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Re-adding support for fractional seconds in access log [tomcat]
jose-galvez commented on PR #720: URL: https://github.com/apache/tomcat/pull/720#issuecomment-2063746801 It can, but in this case the documentation states that it didn't so I was trying to bring it back to how it behaved before (which as an added bonus helps us not change our scripts). If the answer is that this is the correct behavior and the documentation is the one that should change it's fine, we'll figure out a way to adapt or patch it on our end. I just figured it was worth sharing since maybe this specific change was not intended -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Re-adding support for fractional seconds in access log [tomcat]
ChristopherSchultz commented on PR #720: URL: https://github.com/apache/tomcat/pull/720#issuecomment-2063733345 So nothing can ever change, then? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Re-adding support for fractional seconds in access log [tomcat]
jose-galvez commented on PR #720: URL: https://github.com/apache/tomcat/pull/720#issuecomment-2061838297 > Why not use `%D` which is in milliseconds (Tomcat <10) or microseconds (Tomcat >= 10)? I wanted to keep parity on the behavior here with previous versions, especially because in our case we have scripts that use this value for monitoring and are expecting this format. Updating them could be complicated since we would need to support both the Tomcat 9 and Tomcat 10+ format, and also some of our customers have their own scripts that would need to be updated too so it makes things complicated -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Re-adding support for fractional seconds in access log [tomcat]
jose-galvez commented on PR #720:
URL: https://github.com/apache/tomcat/pull/720#issuecomment-2061821679
> From memory that change was to align Tomcat's access log configuration
with httpd. It may well be that the correct fix here is to correct the
documentation. Separately, comparing this PR and the original code I suspect
that, if we did want to merge something along these lines, that there would be
performance concerns with the current proposal. As a minimum we'd need to see
some performance figures for the proposed formatting.
The documentation from Tomcat 9 does indicate that there will be
consolidation but it indicates that it only affects AccessLogValve, not the
ExtendedAccessLogValve (see [this
doc](https://tomcat.apache.org/tomcat-9.0-doc/config/valve.html#Access_Log_Valve)
for %T param).
I could make the change for it to look closer to what it used to do in
Tomcat 9, but I felt this was easier to read and follow while at the same time
wouldn't make a huge performance impact but if that'a a concern I could set it
to do it this way:
```
buf.append(Long.toString(time / 10));
buf.append('.');
int remains = (int) (time % 10);
buf.append(Long.toString(remains / 1));
remains = remains % 1;
buf.append(Long.toString(remains / 1000));
buf.append(Long.toString(remains % 1000));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Re-adding support for fractional seconds in access log [tomcat]
ChristopherSchultz commented on PR #720: URL: https://github.com/apache/tomcat/pull/720#issuecomment-2061771435 Why not use `%D` which is in milliseconds (Tomcat <10) or microseconds (Tomcat >= 10)? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Re-adding support for fractional seconds in access log [tomcat]
markt-asf commented on PR #720: URL: https://github.com/apache/tomcat/pull/720#issuecomment-2061540617 From memory that change was to align Tomcat's access log configuration with httpd. It may well be that the correct fix here is to correct the documentation. Separately, comparing this PR and the original code I suspect that, if we did want to merge something along these lines, that there would be performance concerns with the current proposal. As a minimum we'd need to see some performance figures for the proposed formatting. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
