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

Log all REST api calls (when running as OSGi) #933

Closed
wants to merge 1 commit into from

Conversation

aledsage
Copy link
Contributor

When we switched to OSGi (and thus jax-rs), we no longer logged the REST api calls being made to Brooklyn (see https://github.com/apache/brooklyn-server/blob/master/rest/rest-server/src/main/java/org/apache/brooklyn/rest/filter/LoggingFilter.java for the old way we logged these).

This PR adds a similar logging class for jax-rs.

Copy link
Contributor

@duncangrant duncangrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andreaturli
Copy link
Contributor

lgtm, merging now

@andreaturli
Copy link
Contributor

merged at apache/brooklyn-server master

@aledsage
Copy link
Contributor Author

The diff of file rest/rest-resources/src/main/java/49bd6c7 has been merged to master.

However, this branch (and thus PR) was slightly changed compared to what was merged. The diff compared to master of org/apache/brooklyn/rest/filter/LoggingResourceFilter.java is:

76c76
<     private static final Set<String> CENSORED_HEADERS = ImmutableSet.of("Authorization");
---
>     private static final Set<String> CENSORED_HEADERS = ImmutableSet.of("Authorization", "Cookie");
159c159
<         boolean includeHeaders = (responseContext.getStatus() / 100 == 5) || LOG.isDebugEnabled();
---
>         boolean includeHeaders = (responseContext.getStatus() / 100 == 5) || LOG.isTraceEnabled();

String userName = (userPrincipal != null) ? userPrincipal.getName() : "<no-user>";
String remoteAddr = servletRequest.getRemoteAddr();

StringBuilder message = new StringBuilder("Request received: ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth adding a correlation value here and in the response, so the two can be easily related to each other? Maybe just the hashCode() of the servletRequest object?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geomacy @aledsage would it be useful/possible to use this library http://www.baeldung.com/spring-cloud-sleuth-single-application or at least re-use some design ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think that looks like it's pretty closely tied to Spring. I'm guessing it works in with Spring's request handling. Certainly some of the examples on that page use Spring wiring.

@aledsage aledsage closed this Jan 17, 2018
@aledsage aledsage deleted the log-REST-calls branch January 17, 2018 15:44
@m4rkmckenna
Copy link
Member

@aledsage Sorry im late to the party ... CXF has this functionality built in off the top of my head

Logging[In,Out]Inteceptor just needs to be added to the cxf inteceptor chain

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