[MGNLREST-259] Missing content type response for .rest/status services health check Created: 08/Jul/20 Updated: 10/Jul/20 Resolved: 10/Jul/20 |
|
| Status: | Closed |
| Project: | Magnolia REST Framework |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 2.2.1 |
| Type: | Bug | Priority: | Neutral |
| Reporter: | Viet Nguyen | Assignee: | Maxime Michel |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | maintenance | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Template: |
|
||||||||
| Acceptance criteria: |
Empty
|
||||||||
| Task DoD: |
[ ]*
Doc/release notes changes? Comment present?
[ ]*
Downstream builds green?
[ ]*
Solution information and context easily available?
[ ]*
Tests
[ ]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
||||||||
| Bug DoR: |
[ ]*
Steps to reproduce, expected, and actual results filled
[ ]*
Affected version filled
|
||||||||
| Date of First Response: | |||||||||
| Sprint: | Maintenance 15 | ||||||||
| Description |
|
Customers using REST services health check seeing that the server log was flooded by multiple warnings which telling this service response have no content type set. |
| Comments |
| Comment by Maxime Michel [ 08/Jul/20 ] |
|
Sorry about that. The endpoint does produce a content-type: https://git.magnolia-cms.com/projects/MODULES/repos/rest/browse/magnolia-rest-services/src/main/java/info/magnolia/rest/service/status/StatusEndpoint.java#68 Is it maybe not picked up because the body is empty? Somebody would need to investigate it thorougly. |
| Comment by Maxime Michel [ 09/Jul/20 ] |
|
First observation: returning any content ("hello world") in the response stops the ContentTypeFilter from issuing a warning. |
| Comment by Maxime Michel [ 09/Jul/20 ] |
|
I see people suggest that text/plain rather than application/json should be used for empty responses, as an empty body is not valid JSON, but it's valid text. This however doesn't solve the issue. |
| Comment by Viet Nguyen [ 09/Jul/20 ] |
|
We could also consider giving more informative response in case of OK, instead of empty string. Such as free heapsize or cpu%, just my 2 cents |
| Comment by Maxime Michel [ 09/Jul/20 ] |
|
org.jboss.resteasy.core.ServerResponseWriter#getResponseMediaType returns null when no entity (content) is provided. So either we provide dummy content, or we add a ContentTypeFilter bypass. |
| Comment by Maxime Michel [ 09/Jul/20 ] |
|
That's a good idea viet.nguyen. Let me discuss that with SREs and core developers, and see what we would like to do there. |
| Comment by Maxime Michel [ 09/Jul/20 ] |
|
After discussions, I stumbled upon DEV-1545. The intent of that ticket is to provide customizable parameters that the health check can return. I'm told a health check's utility depends largely on the project. So I would wait for that ticket's research before adding CPU or RAM usage here. Instead, to fix this ticket, I would either respond {}, or add a ContentTypeFilter bypass. |
| Comment by Espen Jervidalo [ 09/Jul/20 ] |
|
Running `curl -vv http://localhost:8080/.rest/status` I get this in the logs: 2020-07-09 16:28:24,670 WARN info.magnolia.cms.filters.ContentTypeFilter : Response is not committed yet. Setting content type: text/html. 2020-07-09 16:28:46,682 WARN info.magnolia.cms.filters.ContentTypeFilter : Content type for http://localhost:8080/.rest/status is not set. In ContentTypeFilter log.warn("Content type for {} is not set.", originalUrl); if (!response.isCommitted()) { //if the content type was not set (e.g. in renderer) and response is not committed, set content type so we don't produce a response without the content type log.warn("Response is not committed yet. Setting content type: {}.", mimeType); response.setContentType(mimeType); } I don't know to what extent it's necessary to set a fallback at all. But logging it on each and every request seems unnecessary. I don't know what best practice is either. But those warns and errors and infos per request spoil the logs. Maybe it's the log aggregator's job to filter/consolidate that.. :shrug: Another thing: When opening the endpoint in the browser it prompts a download dialog for an empty file. |