Reduce log clutter from non-errors

[computerchemist]

Prevent "No matching records" from filling up logfile in terse mode.
#5706

[geographika]

@computerchemist - Travis is failing due to a separate issue. I have changed the pull request to be on the master branch where Travis should be working. If you add in the braces Travis should restart automatically.

[geographika]

@computerchemist - thanks for the pull request!
I may have been mistaken about why Travis was failing. It looks like some tests may be relying on that text e.g. https://github.com/mapserver/mapserver/blob/master/msautotest/wxs/expected/wfs_200_exception_getfeaturebyid_nofeature.xml
Hopefully others will provide input on if this should be expected or not.

[computerchemist]

@geographika I had a quick look, doesn't the context of the test just needs changing so the test is performed in verbose debug mode? The returned WMS makes it clear in any case there are no features to return - that should be really more the focus of the test - the log item itself was IMO pretty useless as it was just telling me "something" had no features. But NOT seeing it repeated literally tens of thousands of times in a production log was the aim of the patch!

I also double checked talking of context to ensure "lp" was there in each function; just something that alarmed me from Travis; warning: ‘lp’ may be used uninitialized in this function. It is - it's initialised via GET_LAYER from the mapObj "map".

I suspect I might be better off using "map->debug", becuase "map" as a mapobj seems to be passed to each function (and DEBUG I think is a global function anyway - is it even able to be turned on for selected layers?). Does that sound like a better idea in any case?

I simply used "lp->debug" as existing error messages were using it. It made ME do a review too and on that basis I think I only got away with it as it's in the layerobj structure, although it all seems to be passed from the mapobj using GET_LAYER, so maybe I'm just confusing travis more than myself.

I'm hoping others can weigh in too at this point who are more knowledgeable with the codebase - all I want is nice clean manageable error logs with only real errors in them!

Thanks for the help again Seth.

[computerchemist]

OK, having delved in deeper I concur this patch does indeed partially suppress the returned message from WMS as well as suppressing the log - in that no "service exception" reason text is given. It doesn't break the spec - indeed it still outputs the correct formatted response as a service exception enough that openlayers sees it as a valid "nothing to do".

The problem seems to stem from the fact that msSetError not only writes the string to the logfile, but also pushes that string via msInsertErrorObj to the exception report in WMS that is thrown after this line by returning MS_FAILURE (which we still need).

As such, I suspect it's probably easier to quietly drop the whole thing, and I'll carry on making my five amendments each time a new mapserver comes out?

The dilemma as I still see it, is this isn't a real "error" - but a "reason" for not outputting something, so it really shouldn't be polluting the error logs on a production server. Here's how the patched version looks when querying via mouse click from openlayers:

<?xml version='1.0' encoding="UTF-8" standalone="no" ?>
<ServiceExceptionReport version="1.3.0" xmlns="http://www.opengis.net/ogc" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.opengis.net/ogc http://schemas.opengis.net/wms/1.3.0/exceptions_1_3_0.xsd">
<ServiceException>
</ServiceException>
</ServiceExceptionReport> 

[geographika]

@computerchemist - a good summary of the situation, it looked a nice easy update at the beginning! I agree with your logic that it should be possible to log only errors in a production system.
I put in link in the mailing lists as there was an ongoing discussion about DEBUG levels which seemed relevant - https://lists.osgeo.org/pipermail/mapserver-users/2018-November/080870.html
The pull request should be left open to let others add opinions.

[jmckenna]

I believe we should merge this pull request, as it is indeed very useful. (just for reference: those log messages will then require at least debug level 3, "verbose").

[jmckenna]

thank you @computerchemist !! A nice improvement.

[ravhed]

I think it was a mistake to merge this pull request into master in its current state as the tests are failing because of this pull request (they currently depend on this error message). It seems to me that there was no decision on how to handle this change so maybe we should revert it and decide on how to properly handle this? I think @geographika's email (https://lists.osgeo.org/pipermail/mapserver-users/2018-November/080870.html) summarizes the issue nicely.

[sdlime]

@jmckenna, @geographika - what do you think? Could just fix the tests. I don't know which or how many tests are failing now but is it simply a matter of adding the appropriate debug level to those tests which depend on that messaging?

[geographika]

The tests need to be updated - we don't want to get into a situation of allowing a few tests to fail.

The ServiceExceptionReport could be improved by including a simple text in the XML string of:

No matching records

[jmckenna]

No problem, will revert now....

[jbo-ads]

I am working on an update of failing msautotests with respect to this PR. It consists in adding a DEBUG 3 line in all impacted layers so that output remains the same.

See #5823.

[jmckenna]

thanks @jbo-ads i was going to tackle that this afternoon, you beat me to it :)

[geographika]

I think with the update from @jbo-ads this pull request can be reopened and merged?