-
Notifications
You must be signed in to change notification settings - Fork 33
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
Stackdriver logging #68 #81
Changes from 12 commits
e748691
f6d1488
2ad99b0
30b3e80
7ec8ff3
1d3ca8a
3b5c3a7
aa81e13
141bae3
48df809
1e38c88
f19135d
4a0886e
df430d6
fe02767
2005182
3d2baec
563140f
3206abf
9de4fc1
f884043
9ea5c97
abc7b09
9318aa1
2dc88d0
d50f5ae
9e5e2db
c6d8df8
d5f5613
9a22c3d
4f738b7
34e9103
f255ee1
fc0ed8d
24a9055
9182d7e
688ed8e
b4d7add
d3e5d9d
06b1a2e
9f64c08
8fb26f2
22e7518
e7a8fcb
4032481
80ee54e
4835abc
f1e55dc
64a0a20
cb4b5f8
cb852d3
f3ef369
72636e8
912d09d
eb6eddb
1976930
2c022d7
cd8bdf8
6ad4bd8
9080fc7
63d986d
cda775a
57613d6
edc450d
c1225ec
1c4c5a9
3a2bfa9
1fc1244
974bb28
7a76e45
c58bbaf
c3d5f0d
17e3af9
87cf4ae
8ea1238
4766ae5
aafc67a
441a3fb
0def22a
81014f6
90ca817
bdad80a
d9a19a9
bc4d478
3240d37
0fc1215
42ba89c
1c73bb6
21fe0cd
151d496
a7e7f37
5a57c6d
d1d9a47
86f8154
e75419d
3f32e54
37f5bd4
4754bc4
8615410
115759a
aa91b0b
91a62d8
2f7dc9c
ac00fd1
187cac8
376481f
4b80727
ae6b9ce
21404d7
de07ed9
df47a07
bfc9e45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
/* | ||
* Copyright 2016 Google Inc. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except | ||
* in compliance with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License | ||
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express | ||
* or implied. See the License for the specific language governing permissions and limitations under | ||
* the License. | ||
*/ | ||
|
||
package com.google.cloud.runtimes.jetty9; | ||
|
||
import org.eclipse.jetty.server.Request; | ||
import org.eclipse.jetty.server.handler.ContextHandler; | ||
import org.eclipse.jetty.server.handler.ContextHandler.Context; | ||
|
||
import java.util.ArrayDeque; | ||
import java.util.Deque; | ||
import java.util.logging.Level; | ||
import java.util.logging.Logger; | ||
|
||
public class RequestContextScope implements ContextHandler.ContextScopeListener { | ||
static final Logger logger = Logger.getLogger(RequestContextScope.class.getName()); | ||
|
||
private static final String X_CLOUD_TRACE = "x-cloud-trace-context"; | ||
private static final ThreadLocal<String> traceid = new ThreadLocal<>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use camel case |
||
private static final ThreadLocal<Deque<Request>> requestStack = | ||
new ThreadLocal<Deque<Request>>() { | ||
@Override | ||
protected Deque<Request> initialValue() { | ||
return new ArrayDeque<>(); | ||
} | ||
}; | ||
|
||
@Override | ||
public void enterScope(Context context, Request request, Object reason) { | ||
if (request != null) { | ||
Deque<Request> stack = requestStack.get(); | ||
if (stack.isEmpty()) { | ||
String id = (String) request.getAttribute(X_CLOUD_TRACE); | ||
if (id == null) { | ||
id = request.getHeader(X_CLOUD_TRACE); | ||
if (id != null) { | ||
int slash = id.indexOf('/'); | ||
if (slash >= 0) { | ||
id = id.substring(0, slash); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we just dropping the span id? I'm not sure this is what we're supposed to be doing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also not sure. The compat version we inherited did a scan for the first non hex character and split there, so I just simplified and inherited that behaviour. Can you investigate if we can leave it in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will find out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got a response from the logging team indicating that we should only log the trace id without the span id. So, this is good. |
||
} | ||
request.setAttribute(X_CLOUD_TRACE, id); | ||
} | ||
} | ||
traceid.set(id); | ||
} | ||
stack.push(request); | ||
} | ||
if (logger.isLoggable(Level.FINE)) { | ||
logger.fine("enterScope " + context); | ||
} | ||
} | ||
|
||
@Override | ||
public void exitScope(Context context, Request request) { | ||
if (logger.isLoggable(Level.FINE)) { | ||
logger.fine("exitScope " + context); | ||
} | ||
if (request != null) { | ||
requestStack.get().pop(); | ||
} | ||
} | ||
|
||
/** Run a Runnable in the scope of a traceid. | ||
* @param traceid The trace ID | ||
* @param runnable the runnable | ||
*/ | ||
public static void runWith(String traceid, Runnable runnable) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this method used somewhere, or just provided as a utility? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a utility that I thought we might need. I will drop for now as it will be trivial to readd if there ever is a need. |
||
String original = RequestContextScope.traceid.get(); | ||
RequestContextScope.traceid.set(traceid); | ||
try { | ||
runnable.run(); | ||
} finally { | ||
RequestContextScope.traceid.set(original); | ||
} | ||
} | ||
|
||
public static Request getCurrentRequest() { | ||
return requestStack.get().peek(); | ||
} | ||
|
||
public static String getCurrentTraceid() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: |
||
return traceid.get(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
/* | ||
* Copyright 2016 Google Inc. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except | ||
* in compliance with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License | ||
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express | ||
* or implied. See the License for the specific language governing permissions and limitations under | ||
* the License. | ||
*/ | ||
|
||
package com.google.cloud.runtimes.jetty9; | ||
|
||
import static com.google.common.base.MoreObjects.firstNonNull; | ||
|
||
import com.google.cloud.MonitoredResource; | ||
import com.google.cloud.logging.AsyncLoggingHandler; | ||
import com.google.cloud.logging.LogEntry.Builder; | ||
import com.google.cloud.logging.LoggingOptions; | ||
|
||
import org.eclipse.jetty.server.Request; | ||
|
||
import java.util.logging.LogManager; | ||
import java.util.logging.LogRecord; | ||
|
||
/** | ||
* A Google Cloud Logging Handler extended with a request traceid label. | ||
*/ | ||
public class TracingLogHandler extends AsyncLoggingHandler { | ||
|
||
private final ThreadLocal<Boolean> flushing = new ThreadLocal<>(); | ||
private final MonitoredResource monitored; | ||
private final String instanceid; | ||
|
||
/** | ||
* Construct a TracingLogHandler for "jetty.log" | ||
*/ | ||
public TracingLogHandler() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need this class anymore, correct? |
||
this(firstNonNull( | ||
LogManager.getLogManager().getProperty(TracingLogHandler.class.getName() + ".log"), | ||
"jetty.log"), null, null); | ||
} | ||
|
||
/** | ||
* Construct a TracingLogHandler. | ||
* | ||
* @param logName Name of the log | ||
* @param options LoggingOptions to access Google cloud API | ||
* @param resource The resource to log against | ||
*/ | ||
public TracingLogHandler(String logName, LoggingOptions options, MonitoredResource resource) { | ||
super(logName, options, resource); | ||
monitored = MonitoredResource.newBuilder("gae_app") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're sort of overwriting the resource that is passed in here. Maybe create the resource only in the no-arg constructor instead? |
||
.addLabel("project_id", System.getenv("GCLOUD_PROJECT")) | ||
.addLabel("module_id", System.getenv("GAE_SERVICE")) | ||
.addLabel("version_id", System.getenv("GAE_VERSION")) | ||
.build(); | ||
instanceid = System.getenv("GAE_INSTANCE"); | ||
} | ||
|
||
@Override | ||
public synchronized void publish(LogRecord record) { | ||
// check we are not already flushing logs | ||
if (Boolean.TRUE.equals(flushing.get())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a matter of style, but I would save a line and write it as: // check we are not already flushing logs
if (!Boolean.TRUE.equals(flushing.get())) {
super.publish(record);
} |
||
return; | ||
} | ||
super.publish(record); | ||
} | ||
|
||
@Override | ||
protected void enhanceLogEntry(Builder builder, LogRecord record) { | ||
super.enhanceLogEntry(builder, record); | ||
String traceid = RequestContextScope.getCurrentTraceid(); | ||
builder.setResource(monitored); | ||
if (traceid != null) { | ||
builder.addLabel("appengine.googleapis.com/trace_id", traceid); | ||
} else { | ||
Request request = RequestContextScope.getCurrentRequest(); | ||
if (request != null) { | ||
builder.addLabel("http-scheme", request.getScheme()); | ||
builder.addLabel("http-method", request.getMethod()); | ||
builder.addLabel("http-uri", request.getOriginalURI()); | ||
builder.addLabel("http-remote-addr", request.getRemoteAddr()); | ||
} | ||
} | ||
if (instanceid != null) { | ||
builder.addLabel("appengine.googleapis.com/instance_name", instanceid); | ||
} | ||
} | ||
|
||
@Override | ||
public synchronized void flush() { | ||
try { | ||
flushing.set(Boolean.TRUE); | ||
super.flush(); | ||
} finally { | ||
flushing.set(Boolean.FALSE); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,11 @@ | ||
<?xml version="1.0"?> | ||
<!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "http://www.eclipse.org/jetty/configure_9_3.dtd"> | ||
<Configure class="org.eclipse.jetty.webapp.WebAppContext"> | ||
|
||
<!-- Protect app.yaml --> | ||
<Get id="protected" name="ProtectedTargets"/> | ||
<Set name="ProtectedTargets"> | ||
<Call class="org.eclipse.jetty.util.ArrayUtil" name="addToArray"> | ||
<Arg><Ref refid="protected"/></Arg> | ||
<Arg>/app.yaml</Arg> | ||
<Arg></Arg> | ||
</Call> | ||
</Set> | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spaces on empty line? |
||
<!-- Add listener to capture request context to be used by logging --> | ||
<Call name="addEventListener"> | ||
<Arg><New class="com.google.cloud.runtimes.jetty9.RequestContextScope"/></Arg> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add this listener conditionally based on an environment variable? For example, if the image is used outside of GCP, this won't make sense. |
||
</Call> | ||
|
||
</Configure> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
.level = INFO | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does one overwrite this configuration? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently they have to replace the file in a Custom image. However, as we discussed on the call last week, we should look at some way of allowing a logging.properties file to be included in the application, to either be merged with this one or simply to replace it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At a minimum, we need to make sure that this still works to overwrite the logging configuration:
See discussion in issue #101 Also, maybe update the README.md as part of this PR specifying how to overwrite the default JUL config. |
||
io.netty.level = INFO | ||
io.gcr.level = INFO | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to set log levels for netty and gcr here if we have the flushing guard in TracingLogHandler.publish()? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added it because netty logs from background threads. However there is still the issue that not all of netty's log mechanism appears not to see these settings anyway, so perhaps we just need an explicit filter in the publish method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would an explicit filter work so that it deals with the Netty background threads? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently the approach taken by the gcloud-java client library is to have fixed list of known log names that it explicitly scans for and removes the stackdriver handler from. This is the approach that should also stop looping, but it not. The same list could simply be applied within the log handler and any logs from those handlers would be discarded. This list could either be a complete list, in which case we would not need either the threadlocal approach nor the handler deregistration. Alternately the list can just be limited to the loggers that are not reacting to the logging properties configuration. Currently I don't like any solution. My preference would be to have no filtering and just loop protection. Overly verbose logging should be handled gracefully (with discarded logs and a message to that effect) and then the user can just configure logging to only log what they are interested in. So I need to investigate why those netty classes are ignoring the properties file. |
||
|
||
handlers = com.google.cloud.runtimes.jetty9.TracingLogHandler | ||
com.google.cloud.runtimes.jetty9.TracingLogHandler.level = INFO | ||
|
||
# handlers = com.google.cloud.runtimes.jetty9.TracingLogHandler,java.util.logging.ConsoleHandler | ||
# java.util.logging.ConsoleHandler.level = INFO |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
// | ||
// ======================================================================== | ||
// Copyright (c) 1995-2016 Mort Bay Consulting Pty. Ltd. | ||
// ------------------------------------------------------------------------ | ||
// All rights reserved. This program and the accompanying materials | ||
// are made available under the terms of the Eclipse Public License v1.0 | ||
// and Apache License v2.0 which accompanies this distribution. | ||
// | ||
// The Eclipse Public License is available at | ||
// http://www.eclipse.org/legal/epl-v10.html | ||
// | ||
// The Apache License v2.0 is available at | ||
// http://www.opensource.org/licenses/apache2.0.php | ||
// | ||
// You may elect to redistribute this code under either of these licenses. | ||
// ======================================================================== | ||
// | ||
|
||
package com.google.cloud.runtimes.jetty9; | ||
|
||
import java.io.IOException; | ||
import java.util.logging.Logger; | ||
|
||
import javax.annotation.PostConstruct; | ||
import javax.servlet.ServletException; | ||
import javax.servlet.annotation.WebServlet; | ||
import javax.servlet.http.HttpServlet; | ||
import javax.servlet.http.HttpServletRequest; | ||
import javax.servlet.http.HttpServletResponse; | ||
|
||
|
||
@SuppressWarnings("serial") | ||
@WebServlet(urlPatterns = {"/", "/test/*"}, name = "TestServlet") | ||
public class TestServlet extends HttpServlet { | ||
static final Logger log = Logger.getLogger(TestServlet.class.getName()); | ||
|
||
@PostConstruct | ||
private void myPostConstructMethod() { | ||
log.info("preconstructed"); | ||
} | ||
|
||
@Override | ||
public void init() throws ServletException { | ||
log.info("init info"); | ||
getServletContext().log("init ServletContext.log"); | ||
} | ||
|
||
public void doGet(HttpServletRequest request, HttpServletResponse response) | ||
throws ServletException, IOException { | ||
log.info("doGet info"); | ||
getServletContext().log("doGet ServletContext.log"); | ||
|
||
if (request.getParameter("ex") != null) { | ||
try { | ||
throw (Throwable) Class.forName(request.getParameter("ex")).newInstance(); | ||
} catch (ServletException | IOException e) { | ||
throw e; | ||
} catch (Throwable e) { | ||
throw new ServletException(e); | ||
} | ||
} | ||
response.getWriter().println("Log Test"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some javadocs to explain the purpose of the class?