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

SOLR-14142 Enable jetty's request log by default #536

Merged
merged 11 commits into from
Jan 18, 2022

Conversation

janhoy
Copy link
Contributor

@janhoy janhoy commented Jan 18, 2022

(SOLR_REQUESTLOG_ENABLED defaults to true)

https://issues.apache.org/jira/browse/SOLR-14142

Picking up this old jira, hoping to get it into 9.0. This PR is the exact same as Rob's latest patch in JIRA. I have not tested it but let's do another review.

There were some concerns about the requestlog not being rotated, and filling up disks. If it is a quick fix with some sys prop then let's add a default of 1Gb or something. There should perhaps be a RefGuide mention as well?

Copy link
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

I think the Windows change is fine (it is just copypaste from the setting afterwards).

This PR does not yet have a limit on request log length? I have no idea how to enable that in Jetty. I generally use Embedded Jetty and I feed the requestlog through Log4j2 with a custom handler on INFO level (so I have everything in same log). To disable you can do this in the log4j.xml/properties file. Or redirect the request log to another file. My handler also sets MDC thread context variables with the important infos like method/uri/user-agent/...

We may also go that route in future. Only that the log format is then not "standard NCSA httpd format".

@uschindler
Copy link
Contributor

uschindler commented Jan 18, 2022

For the record this is how to use log4j (just some debugging example copypasted):

static final class LoggingAccessLog extends AbstractLifeCycle implements RequestLog {
  private final org.apache.logging.log4j.Logger log;
  
  public LoggingAccessLog(String logname) {
    this.log = org.apache.logging.log4j.LogManager.getLogger(logname);
  }
  
  @Override
  public void log(Request req, Response resp) {
    final StringBuilder line=new StringBuilder("From: ")
      .append(req.getRemoteHost())
      .append("; Req: \"").append(req.getMethod()).append(' ').append(req.getRequestURI());
    final String query = req.getQueryString();
    if (query != null) {
      line.append('?').append(query);
    }
    line.append(' ').append(req.getProtocol()).append("\"; Status: ").append(resp.getStatus()).append("; Bytes: ").append(resp.getContentCount());
    final String referer=req.getHeader(HttpHeader.REFERER.asString());
    if (referer!=null) line.append("; Referer: \"").append(referer).append('"');
    final String userAgent=req.getHeader(HttpHeader.USER_AGENT.asString());
    if (userAgent!=null) line.append("; User-Agent: \"").append(userAgent).append('"');
    if (req.isSecure()) line.append("; secure");
    log.info(line.toString());
  }
}

final RequestLogHandler requestLogHandler=new RequestLogHandler();
requestLogHandler.setRequestLog(new LoggingAccessLog("web-access"));
allJettyHandlers.addHandler(requestLogHandler);

Copy link
Member

@rmuir rmuir left a comment

Choose a reason for hiding this comment

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

Thanks! We gotta start with something!

I strongly suggest keeping one the NCSA standardized formats and not doing some custom format: it is really important to use a standardized format if the app is just gonna create files, otherwise its too hard for anyone to parse/forward the data for analysis or aggregation. For example, in this case because it is standardized, I can e.g. just point splunk at the file and go.

If anyone has a serious high-volume use-case, then they are surely doing TLS termination and have a load balancer and can produce/forward/filter/tweak these logs in some custom way there, and simply disable this solr request log.

@janhoy
Copy link
Contributor Author

janhoy commented Jan 18, 2022

Added a major changes entry in ref guide, please review.

I plan to test this one of these days and then commit to all branches, if no furhter comments. Feel free to commit doc fixes directly to the pr branch, if any.

Copy link
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

All fine thanks for documentation updates!

@janhoy
Copy link
Contributor Author

janhoy commented Jan 18, 2022

I tested with no VAR -> log enabled, set to true -> log enabled, set ot false -> no requestlog.
Checked basic functionality.
Tried to test deletion of old logs by creating several old logs, but they do not get deletes at startup, only on rollover at midnight.
All in all - looks good, will merge.

@janhoy janhoy merged commit 1d761d2 into apache:main Jan 18, 2022
@janhoy janhoy deleted the SOLR14142-requestlog-default-on branch January 18, 2022 23:10
janhoy added a commit that referenced this pull request Jan 18, 2022
janhoy added a commit that referenced this pull request Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants