[Geoserver-devel] gwc 304s treated as errors

Rob_Marianski · December 18, 2013, 4:28pm

I noticed that the monitoring extension is classifying Geowebcache
cache hits as errors. The reason this happens is that on a cache
hit, Geowebcache throws an HttpErrorCodeException (with a 304 code).
The dispatcher catches this, and sets the error onto the request,
which the monitoring module filter picks up. Here's a link to the
relevant part of the Dispatcher:
https://github.com/geoserver/geoserver/blob/a7eff115e4343163dc99a88c6e849db85ffa981d/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java#L1603

I can think of a few different possible places to try and resolve
this, in order of intrusiveness:

1. In the Dispatcher, explicitly check to see that the
HttpErrorCodeException error code is >= 400 before assigning the error
to the request. If code < 400, don't set the error on the request.

2. In the monitoring module MonitorFilter, check to see if the error
is an HttpErrorCodeException. If so, and the code is < 400,
don'tconsider it to be an error.

3. In my case, I have a plugin to the monitoring module, so I can
putthe relevant check there.

I think the change make sense in the dispatcher, because a 304 is not
really an error. But, I'm not sure if something like this would have
any negative consequences, and I'd imagine that any change here could
have far reaching effects.

Below is what that Dispatcher change could look like. Thoughts?

diff --git a/src/ows/src/main/java/org/
geoserver/ows/Dispatcher.java
b/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java
index 54a0719..ef258f5 100644
--- a/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java
+++ b/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java
@@ -1593,6 +1593,11 @@ public class Dispatcher extends AbstractController {
                } else {
                        request.getHttpResponse().sendError(ece.getErrorCode());
                }
+ if (ece.getErrorCode() < 400) {
+ // gwc returns an HttpErrorCodeException for 304s
+ // we don't want to flag these as errors for
upstream filters, ie the monitoring extension
+ t = null;
+ }
             }
             catch (IOException e) {
                 //means the resposne was already commited

--
Robert Marianski
Software Engineer | Boundless
rmarianski@anonymised.com
@boundlessgeo

Justin_Deoliveira · January 6, 2014, 5:42pm

Hey Rob,

Not sure if you got any resolution on this, and perhaps went with option 3 but i’ll weigh in with my thoughts regardless.

I agree that option 1 makes the most sense, since I can’t think of a case where the dispatcher would want to classify anything with a code < 400 an error. But perhaps one of the other devs can think of one.

If we wanted to be ultra careful we could apply the change to the dispatcher on master, and on the stable branches apply the change in the MonitorFilter. Personally I would be fine with option 1 on both master and stable.

-Justin

···

On Wed, Dec 18, 2013 at 9:28 AM, Rob Marianski <rmarianski@anonymised.com> wrote:

I noticed that the monitoring extension is classifying Geowebcache
cache hits as errors. The reason this happens is that on a cache
hit, Geowebcache throws an HttpErrorCodeException (with a 304 code).
The dispatcher catches this, and sets the error onto the request,
which the monitoring module filter picks up. Here’s a link to the
relevant part of the Dispatcher:
https://github.com/geoserver/geoserver/blob/a7eff115e4343163dc99a88c6e849db85ffa981d/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java#L1603

I can think of a few different possible places to try and resolve
this, in order of intrusiveness:

In the Dispatcher, explicitly check to see that the
HttpErrorCodeException error code is >= 400 before assigning the error
to the request. If code < 400, don’t set the error on the request.

In the monitoring module MonitorFilter, check to see if the error
is an HttpErrorCodeException. If so, and the code is < 400,
don’tconsider it to be an error.

In my case, I have a plugin to the monitoring module, so I can
putthe relevant check there.

I think the change make sense in the dispatcher, because a 304 is not
really an error. But, I’m not sure if something like this would have
any negative consequences, and I’d imagine that any change here could
have far reaching effects.

Below is what that Dispatcher change could look like. Thoughts?

diff --git a/src/ows/src/main/java/org/
geoserver/ows/Dispatcher.java
b/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java
index 54a0719…ef258f5 100644
— a/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java
+++ b/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java
@@ -1593,6 +1593,11 @@ public class Dispatcher extends AbstractController {
} else {
request.getHttpResponse().sendError(ece.getErrorCode());
}

if (ece.getErrorCode() < 400) {

// gwc returns an HttpErrorCodeException for 304s

// we don’t want to flag these as errors for
upstream filters, ie the monitoring extension

t = null;

}
}
catch (IOException e) {
//means the resposne was already commited

–
Robert Marianski
Software Engineer | Boundless
rmarianski@anonymised.com
@boundlessgeo

Rapidly troubleshoot problems before they affect your business. Most IT
organizations don’t have a clear picture of how application performance
affects their revenue. With AppDynamics, you get 100% visibility into your
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk

Geoserver-devel mailing list
Geoserver-devel@anonymised.comsts.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

–

Justin Deoliveira
Vice President, Engineering | Boundless
jdeolive@anonymised.com
@j_deolive

Rob_Marianski · January 6, 2014, 6:30pm

Ok, I'll create a ticket on jira and work on getting a patch submitted.

Do others also feel that it's ok to have a change to the Dispatcher
land on all the release branches? Or should I work towards 2 patches,
one for the dispatcher that might get only applied to master, and one
for the MonitorFilter that would get applied to the other release
branches?

Robert

On Mon, Jan 6, 2014 at 12:42 PM, Justin Deoliveira
<jdeolive@anonymised.com> wrote:

Hey Rob,

Not sure if you got any resolution on this, and perhaps went with option 3
but i'll weigh in with my thoughts regardless.

I agree that option 1 makes the most sense, since I can't think of a case
where the dispatcher would want to classify anything with a code < 400 an
error. But perhaps one of the other devs can think of one.

If we wanted to be ultra careful we could apply the change to the dispatcher
on master, and on the stable branches apply the change in the MonitorFilter.
Personally I would be fine with option 1 on both master and stable.

-Justin

On Wed, Dec 18, 2013 at 9:28 AM, Rob Marianski <rmarianski@anonymised.com>
wrote:

I noticed that the monitoring extension is classifying Geowebcache
cache hits as errors. The reason this happens is that on a cache
hit, Geowebcache throws an HttpErrorCodeException (with a 304 code).
The dispatcher catches this, and sets the error onto the request,
which the monitoring module filter picks up. Here's a link to the
relevant part of the Dispatcher:

https://github.com/geoserver/geoserver/blob/a7eff115e4343163dc99a88c6e849db85ffa981d/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java#L1603

I can think of a few different possible places to try and resolve
this, in order of intrusiveness:

1. In the Dispatcher, explicitly check to see that the
HttpErrorCodeException error code is >= 400 before assigning the error
to the request. If code < 400, don't set the error on the request.

2. In the monitoring module MonitorFilter, check to see if the error
is an HttpErrorCodeException. If so, and the code is < 400,
don'tconsider it to be an error.

3. In my case, I have a plugin to the monitoring module, so I can
putthe relevant check there.

I think the change make sense in the dispatcher, because a 304 is not
really an error. But, I'm not sure if something like this would have
any negative consequences, and I'd imagine that any change here could
have far reaching effects.

Below is what that Dispatcher change could look like. Thoughts?

diff --git a/src/ows/src/main/java/org/
geoserver/ows/Dispatcher.java
b/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java
index 54a0719..ef258f5 100644
--- a/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java
+++ b/src/ows/src/main/java/org/geoserver/ows/Dispatcher.java
@@ -1593,6 +1593,11 @@ public class Dispatcher extends AbstractController
{
                } else {

request.getHttpResponse().sendError(ece.getErrorCode());
                }
+ if (ece.getErrorCode() < 400) {
+ // gwc returns an HttpErrorCodeException for 304s
+ // we don't want to flag these as errors for
upstream filters, ie the monitoring extension
+ t = null;
+ }
             }
             catch (IOException e) {
                 //means the resposne was already commited

--
Robert Marianski
Software Engineer | Boundless
rmarianski@anonymised.com
@boundlessgeo

------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT
organizations don't have a clear picture of how application performance
affects their revenue. With AppDynamics, you get 100% visibility into your
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics
Pro!

http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

--
Justin Deoliveira
Vice President, Engineering | Boundless
jdeolive@anonymised.com
@j_deolive

--
Robert Marianski
Software Engineer | Boundless
rmarianski@anonymised.com
@boundlessgeo