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

Make internal logs accessible via REST API call #1452

Merged
merged 12 commits into from Oct 6, 2015

Conversation

Projects
None yet
3 participants
@lennartkoopmann
Member

lennartkoopmann commented Sep 28, 2015

  • Log4J appender writing logs to a fixed size in memory destination
  • REST API call to access the logs

This allows Apollo to collect the most recent logs and also the graylog-web-interface to display server logs. This will make maintenance easier and our support faster.

Make internal logs accessible via REST API call
* Log4J appender writing logs to a fixed size in memory destination
* REST API call to access the logs

This allows Apollo to collect the most recent logs and also the graylog-web-interface to display server logs. This will make maintenance easier and our support faster.

@lennartkoopmann lennartkoopmann added this to the 1.x milestone Sep 28, 2015

* along with Graylog. If not, see <http://www.gnu.org/licenses/>.
*/
package org.graylog2;

This comment has been minimized.

@lennartkoopmann

lennartkoopmann Sep 28, 2015

Member

I was not sure about the location but this like felt the best.

This comment has been minimized.

@dennisoelkers

dennisoelkers Sep 29, 2015

Member

It really feels misplaced there. Maybe put it in an own package?

This comment has been minimized.

@joschi

joschi Sep 29, 2015

Contributor

org.graylog.log4j would work, IMHO. It's not really bound to Graylog in any way, so we could even create a separate artifact for it. :trollface:

@dennisoelkers dennisoelkers self-assigned this Sep 29, 2015

@AutoValue
@JsonAutoDetect
public abstract class LogMessage {

This comment has been minimized.

@dennisoelkers

dennisoelkers Sep 29, 2015

Member

LogMessage is too generic and too easy to mix up with the Message class. Please use something like InternalLogMessage at least if you can't come up with a more precise name.

package org.graylog2;
import com.google.common.collect.Lists;
import edu.emory.mathcs.backport.java.util.Arrays;

This comment has been minimized.

@dennisoelkers

dennisoelkers Sep 29, 2015

Member

Wrong import, please use java.util.Arrays

import javax.ws.rs.core.MediaType;
import java.util.Enumeration;
import java.util.Map;
import java.util.*;

This comment has been minimized.

@dennisoelkers

dennisoelkers Sep 29, 2015

Member

Please don't use wildcard imports.

This comment has been minimized.

@lennartkoopmann

lennartkoopmann Oct 4, 2015

Member

I think it makes sense to have a wildcard import here given all the java.util imports that happen. Actually IntelliJ keeps overwriting it to a wildcard. Namespace cluttering/confusion from java.util is not expected IMO.

This comment has been minimized.

@joschi

joschi Oct 5, 2015

Contributor

We're not using wildcard imports at all in the Graylog codebase (as part of our informal coding standards). See http://stackoverflow.com/a/3348855/49505 for how to disable that in IntelliJ IDEA.

This comment has been minimized.

@lennartkoopmann
})
@Path("/messages/recent")
@Produces(MediaType.APPLICATION_JSON)
public LogMessagesSummary messages(@ApiParam(name = "max", required = true) @QueryParam("max") int max) {

This comment has been minimized.

@dennisoelkers

dennisoelkers Sep 29, 2015

Member

The commonly used parameter for a maximum number of elements to return in our REST resources is "limit", please use that instead of "max".

This comment has been minimized.

@joschi

joschi Sep 29, 2015

Contributor

Alternatively, we use "size" in different places as well.

MemoryAppender appender = (MemoryAppender) logger.getAppender("memory");
for (LoggingEvent loggingEvent : appender.getLogMessages(max)) {
List<String> throwable = new ArrayList<>();

This comment has been minimized.

@dennisoelkers

dennisoelkers Sep 29, 2015

Member

Why are you initializing a new ArrayList here just to overwrite it soon after? If it's for the purpose of using an empty list in case of getThrowableStrRep() returning null, then doing that in an else-clause and using Collections.emptyList() would be better.

public abstract String className();
@JsonProperty
public abstract String level();

This comment has been minimized.

@joschi

joschi Sep 29, 2015

Contributor

Using an enum here would be better.

This comment has been minimized.

@lennartkoopmann

lennartkoopmann Oct 4, 2015

Member

an enum for what? level?

This comment has been minimized.

@joschi

joschi Oct 5, 2015

Contributor

Yes, exactly. Either use Level or write your own enum. This way you don't have to juggle with that stringly typed API. 😉

public abstract String level();
@JsonProperty
public abstract String timestamp();

This comment has been minimized.

@joschi

joschi Sep 29, 2015

Contributor

Why not DateTime?

}
List<LoggingEvent> messages = Lists.newArrayList();
ReverseListIterator iter = new ReverseListIterator(new ArrayList<LoggingEvent>(Arrays.asList(buffer.toArray())));

This comment has been minimized.

@joschi

joschi Sep 29, 2015

Contributor

TooManyCollectionsException…

    public List<LoggingEvent> getLogMessages(int max) {
        if (buffer == null) {
            throw new IllegalStateException("Cannot return log messages: Appender is not initialized.");
        }

        final List<LoggingEvent> result = new ArrayList<>(max);
        final Object[] messages = buffer.toArray();
        for(int i = messages.length - 1; i >= 0 && i >= messages.length - max; i--) {
            result.add((LoggingEvent) messages[i]);
        }

        return result;
    }

This comment has been minimized.

@lennartkoopmann
ReverseListIterator iter = new ReverseListIterator(new ArrayList<LoggingEvent>(Arrays.asList(buffer.toArray())));
int i = 0;
while(iter.hasNext()) {
messages.add((LoggingEvent) iter.next());

This comment has been minimized.

@joschi

joschi Sep 29, 2015

Contributor

This will cause an off-by-1 error if max == 0.

Logger logger = Logger.getRootLogger();
if (logger.getAppender("memory") == null) {
throw new WebApplicationException(404);

This comment has been minimized.

@joschi

joschi Sep 29, 2015

Contributor

There's a NotFoundException which automatically sets the correct response status. Additionally, a short explaining message would be nice (e. g. "In-Memory appender not found. Please check the log4j configuration of this Graylog instance.")

throw new WebApplicationException(404);
}
MemoryAppender appender = (MemoryAppender) logger.getAppender("memory");

This comment has been minimized.

@joschi

joschi Sep 29, 2015

Contributor

This should check the actual type of the appender with instanceof first and throw an internal server error if it doesn't fit.

}
public void setBufferSize(int bufferSize) {
this.bufferSize = bufferSize;

This comment has been minimized.

@joschi

joschi Sep 29, 2015

Contributor

This should check for bufferSize >= 0, e. g. with checkArgument.

* A Log4J appender that keeps a configurable number of messages in memory. Used to make recent internal log messages
* available via the REST API.
*/
public class MemoryAppender extends AppenderSkeleton {

This comment has been minimized.

@joschi

joschi Sep 29, 2015

Contributor

Maybe rename to InMemoryAppender.

This comment has been minimized.

@lennartkoopmann

lennartkoopmann Oct 4, 2015

Member

What would be the benefit of InMemoryAppender vs MemoryAppender?

This comment has been minimized.

@joschi

joschi Oct 5, 2015

Contributor

"Naming things is hard". In my opinion, _In_MemoryAppender describes better what this class is doing. But I'm not too opinionated about this.

@@ -10,6 +10,10 @@
</layout>
</appender>
<appender name="memory" class="org.graylog2.MemoryAppender">

This comment has been minimized.

@joschi

joschi Sep 29, 2015

Contributor

The name should be more obviously point to the internal usage of this appender, e. g. "graylog-internal-logs", and have a short comment that this shouldn't be removed or changed.

@lennartkoopmann

This comment has been minimized.

Member

lennartkoopmann commented Oct 4, 2015

Thanks for the review! :) I have addressed/fixed all issues and if not then left a comment at your respective note.

@joschi

This comment has been minimized.

Contributor

joschi commented on graylog2-radio/misc/graylog2-radio.conf in d0dd347 Oct 5, 2015

IntelliJ IDEA Refactoring gone wrong™.

@joschi

This comment has been minimized.

There should be a default value for limit (@DefaultValue), e. g. 500 messages (default buffer size for MemoryAppender).

@Path("/messages/recent")
@Produces(MediaType.APPLICATION_JSON)
public LogMessagesSummary messages(@ApiParam(name = "limit", required = true) @QueryParam("limit") int limit) {
List<InternalLogMessage> messages = new ArrayList<>();

This comment has been minimized.

@joschi

joschi Oct 5, 2015

Contributor

This list could be initialized with the correct size (appender.getLogMessages(limit).size()).

@lennartkoopmann

This comment has been minimized.

Member

lennartkoopmann commented Oct 5, 2015

Thank you, sir! Just pushed a commit that addresses your proposed changes.

})
@Path("/messages/recent")
@Produces(MediaType.APPLICATION_JSON)
public LogMessagesSummary messages(@ApiParam(name = "limit", required = true, defaultValue = "500") @QueryParam("limit") int limit) {

This comment has been minimized.

@joschi

joschi Oct 6, 2015

Contributor

Unfortunately that's not enough, as @ApiParam is just an annotation used by Swagger to generate the documentation/Swagger specification. @DefaultParam is the correct annotation. :trollface:

@dennisoelkers

This comment has been minimized.

This is unnecessary, as LoggingEvent#getProperties() is already calling it.

This comment has been minimized.

Contributor

joschi replied Oct 6, 2015

dennisoelkers added a commit that referenced this pull request Oct 6, 2015

Merge pull request #1452 from Graylog2/logs-rest
Make internal logs accessible via REST API call

@dennisoelkers dennisoelkers merged commit 866d6e3 into 1.2 Oct 6, 2015

3 checks passed

ci Jenkins build graylog2-server-integration-pr 257 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@lennartkoopmann

This comment has been minimized.

Member

lennartkoopmann commented on db4c843 Oct 6, 2015

argh, sorry, I forgot that one :)

@lennartkoopmann lennartkoopmann deleted the logs-rest branch Oct 6, 2015

@dennisoelkers dennisoelkers restored the logs-rest branch Oct 6, 2015

dennisoelkers added a commit that referenced this pull request Oct 6, 2015

Merge pull request #1452 from Graylog2/logs-rest
Make internal logs accessible via REST API call
(cherry picked from commit 866d6e3)

@joschi joschi deleted the logs-rest branch Oct 12, 2015

dennisoelkers added a commit that referenced this pull request Oct 12, 2015

Merge pull request #1452 from Graylog2/logs-rest
Make internal logs accessible via REST API call
(cherry picked from commit 866d6e3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment