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

Render health checks as JSON for ratpack-codahale-metrics #467

Merged
merged 11 commits into from Nov 14, 2014

Conversation

wmacgyver
Copy link

per #416

I kept it simple and just use the Jackson mapper

@ldaley ldaley added this to the release-0.9.10 milestone Oct 16, 2014
@ldaley
Copy link
Member

ldaley commented Oct 16, 2014

@rhart do you mind shepharding this one as you're our metrics expert?

@rhart
Copy link
Member

rhart commented Oct 16, 2014

No prob, will take a look tomorrow

@wmacgyver
Copy link
Author

since I'm being shepherded, Baaaaaa....

@rhart
Copy link
Member

rhart commented Oct 17, 2014

Never say "Baaaa" to a Welshman ;)

@rhart
Copy link
Member

rhart commented Oct 17, 2014

@wmacgyver now we're returning json we should prob be using Context#byContent in the renderer, what do you think? There's an example in DefaultJsonRenderer https://github.com/ratpack/ratpack/blob/master/ratpack-jackson/src/main/java/ratpack/jackson/internal/DefaultJsonRenderer.java#L42

Also, I think we should use Context#getResponse()#send instead of Context#render to avoid another renderer lookup.

@wmacgyver
Copy link
Author

sure, I'll make the changes you suggested tomorrow.

Wilson MacGyver added 2 commits October 27, 2014 18:16
content.getReponse().send to avoid lookup as per requested
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.15%) when pulling 8d9b9c5 on wmacgyver:master into d9242af on ratpack:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.11%) when pulling 8d9b9c5 on wmacgyver:master into d9242af on ratpack:master.

@rhart
Copy link
Member

rhart commented Oct 29, 2014

I'll merge this when I get back from hols. It'll be after 0.9.10

I'm thinking it might be worthwhile explicitly setting "no cache" response headers too. As I understand it you can't guarantee the browser won't cache the response without setting them. Unless you're explicit you're at the mercy of the browser.

@rhart rhart removed this from the release-0.9.10 milestone Oct 29, 2014
@wmacgyver
Copy link
Author

added http header to disable caching. I did all 3 variant to make sure it cover all the browsers. Hope that wasn't too over zealous.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.52%) when pulling 6237d0b on wmacgyver:master into d9242af on ratpack:master.

@rhart rhart added this to the release-0.9.11 milestone Nov 14, 2014
@rhart rhart removed the in progress label Nov 14, 2014
rhart added a commit that referenced this pull request Nov 14, 2014
Render health checks as JSON for ratpack-codahale-metrics
@rhart rhart merged commit 9593dec into ratpack:master Nov 14, 2014
@rhart
Copy link
Member

rhart commented Nov 14, 2014

Sorry it's taken so long but y'know, hols take priority :)

@wmacgyver
Copy link
Author

@rhart no worries, thanks for the hand holding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants