-
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
Implement stackdriver logging #68
Comments
work in progress will be in branch https://github.com/GoogleCloudPlatform/jetty-runtime/tree/master-68 |
@meltsufin currently logging is split over Ultimately, I can see the results of this issue being split over:
I propose that for simplicity, I develop all the code here in OK? |
@gregw sounds good to develop the code here first. |
@meltsufin progress is slow. I'm a bit concerned that the JUL to google-cloud-java approach may be a bit flawed. It is working when the log level is INFO, but once you make the level FINE, then the implementation of google-cloud-java using things like netty and HttpURLConnection that themselves log to java util logging, so infinite recursion and stack overflows result! See googleapis/google-cloud-java#1386 We could go straight from the jetty log to google-cloud-logging, but anything that uses JUL or JCL (eg parts of the datastore used for sessions) will be lost (or end up on the console). It may be that the google-cloud-logging developers have a solution to prevent the recursion. But I think it really needs a very simple HTTP client that does not do logging to be used to talk to the stackdrive service. Perhaps the google-cloud-java library can be used to do the auth first. |
@gregw Can you use a thread local flag to signal to the handler that it's being recursively called and break the infinite recursion? |
@meltsufin we should be able to use a thread local, although care will be needed to ensure that we don't hammer on that for every log line. I've not done that yet, as it is really something that needs to be solved at the google-cloud-java level, but may do so today just to make some progress. I've been thinking over alternate solutions and nothing really is as satisfactory as having a JUL handler receiving all container logs from all sources and sending them to stackdriver |
The LoggingHandler does have protection for looping in it's implementation. It maintains a list of masked loggers and removes the stackdriver logger from them. However, for some reason it is not working for my testing. I am getting assistance to analyse. Good news is that the logs are appearing in the console... just an infinite number of them :) |
@gregw It actually makes sense that the LoggingHandler would have protection against infinite recursion, but I guess the way we're trying to redirect logs is breaking it. Maybe we're somehow capturing the logs it skips and directng them back to it? Anyway, thanks for the update. |
We have not yet found or fixed the log looping problem in googleapis/google-cloud-java#1386, but as they are working on it and as it indicates that loop logging is indeed something the API code will deal with, I don't think this indicates we have an architectural issue. So I will ignore the problem for now (don't set logging to FINE) and finish this task. I'll separately help the google-cloud-java team debug and fix the looping issue. |
Are you ready to create a pull request to start the review process? |
Not just yet. Let me upgrade to the just release RC2 and do a bit more testing. I'd also like to automate some tests, but I don't think we should hold up the PR for that. |
@meltsufin I have the logging working now, however it uses a modified LoggingHandler that uses a threadlocal. So I will not create the PR until googleapis/google-cloud-java#1385 is resolved.... unless you want me to create the PR so we can do an early review and chat about the code? |
@gregw I will look at the code from the branch for now. Thanks! |
@gregw I tried building an image from your branch, but was not able to deploy an app based on this image. Does the branch have your latest change with the use of threadlocal?
|
Oops pushed failed... pushed now (and merged with latest master). On 18 November 2016 at 04:58, meltsufin notifications@github.com wrote:
Greg Wilkins gregw@webtide.com CTO http://webtide.com |
@gregw It's running fine now. I can actually see the logs in the Log Viewer and they have the trace id, but they all appear under the "Global" resource, rather than "GAE Application" -> [service] -> [version]. This is caused by the "resource" not being setup with the necessary metadata in the LogEntry. I wonder if this is something we need to explicitly configure, or the cloud-logging-library should be doing it automatically. |
I'll investigate and ask the google-cloud-java folks if need be. |
@meltsufin I'm trying to add a MonitoredResource, but it is causing all the logs to go missing! I create the resource with monitored = MonitoredResource.newBuilder("gae_app")
.addLabel("project_id", System.getenv("GCLOUD_PROJECT"))
.addLabel("module_id", System.getenv("GAE_SERVICE"))
.addLabel("version_id", System.getenv("GAE_VERSION"))
.addLabel("instance_id", System.getenv("GAE_INSTANCE"))
.build();
System.err.println("MonitoredResource = " + monitored); and when run in env:flex I can see the stderr log in the console reporting:
For each log record, I now add the resource: protected void enhanceLogEntry(Builder builder, LogRecord record) {
super.enhanceLogEntry(builder, record);
String traceid = RequestContextScope.getCurrentTraceid();
builder.setResource(monitored);
if (traceid != null) {
builder.addLabel("traceid", 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());
}
}
} But I cannot find any logs in the console? I'm currently running an instance in Any ideas? |
It's a mystery for me too. Log Viewer might be using some other metadata to organize the logs. I've asked the folks who work on the Log Viewer for help. I'll update when I hear back from them. |
I'm printing out the google API LogEntry as they are generated and they look like:
|
The |
@gregw I got an update from the Logging team, and they're saying that the To get the list of valid labels, you can e.g. use our API https://cloud.google.com/logging/docs/api/reference/rest/v2/monitoredResourceDescriptors/list
|
Ah that was me adding the instance_id as I thought it may be useful. I thought it would just be a label that would be ignored and but that could be filtered on if desired. I will remove and try again (although it could be good information to include somewhere, perhaps on the log label rather than the resource label?) With regards to errors, I think we are not getting any as they should be written to stderr if we did. However I will double check and perhaps induce a few forced errors to double check we can see logs. I'll work on this tomorrow. |
@meltsufin I did some simple memory usage testing, creating logs in a loop and every 1000 I checking the difference in the memory used by the JVM. I get output like:
This suggests that each log is generating about 25KB of garbage! I think this warrants further investigation and I shall try a profiling tool on it next week. |
Thanks Greg! This is obviously way too much garbage, if it's true! Also, this issue with netty memory leaks may be related. googleapis/google-cloud-java#1433 |
No longer needed once GoogleCloudPlatform/jetty-runtime#68 is resolved
No longer needed once GoogleCloudPlatform/jetty-runtime#68 is resolved
* Issue #68 added request context scope * Issue #68 added request context scope * Issue #68 added skeleton Logging Handler * Issue #68 work in progress * Issue #68 work in progress * working implementation using a threadlocal to stop looping * Issue #68 update to latest google-cloud-java LoggingHandler * Issue #68 Updated to latest gcloud API removed copied LoggingHandlers removed instanceid from monitored resource * Issue #68 Use same labels as nginx * Issue #68 Code cleanups after review * Issue #68 Removed stackdriver logging, so this just configures JUL to capture the traceid and log to stderr * renamed traceid to traceId * gcloud api not currently used * Tunnel traceId in parameters #68 * Simplified with traceId lookup in formatter #68 * Use 8.2 beta stackdriver * Improved formatting * use logger name rather than source * run jetty from the webapp working directory so logging configuration may be relative * Testing logging Test googleapis/google-cloud-java#1535 and jetty-9.4.1-SNAPSHOT * use released 9.4.1 * Issue #68 minor clean ups * Do not CD to non existant directory * improved README * fixed Cloud SDK * Released beta of google-cloud-logging * updated google-cloud API to 0.8.3-beta * added remote test to check logging * Stackdriver logging testing improved comments check for traceid * upgrade to 0.9.2 * Test for zone * upgrade to 10.0 gcloud API * enable gae module only of GAE_INSTANCE environment variable is set * improved gae.mod documentation * GCP module Rename gae module and configuration to gcp Split the jetty.commands into jetty.commands and gcp.commands moved commands file to jetty-base/config-scripts updated setup-env-ext to run the gcp.commands when image is run with a GAE_INSTANCE set * fixed warnings * turn off stackdriver logging by default. Added instructions to enable. * Updates Update to latest openjdk-runtime with setup-env.d Update to latest jetty release * Use the PLATFORM env var * Improved jetty startup Added JETTY_PROPERTIES, JETTY_MODULES_ENABLE & JETTY_MODULES_DISABLE Removed duplicate code fron 50-jetty.bash * use launcher URL * Trimmed GCP specific configuration * Added structure tests for jetty setup script Also fixed unpack bug found as a result * Support passing just args * fix merge * Fixed test workspace paths for cloud build * review feedback * working directory is the root webapp * Improve handling of various types of command line. Fixed problem with handling of command line like "ls /var" Requires duplication of test for java from openjdk-runtime docker-entrypoint.bash, which should be modified to avoid the duplication. * upgrade cgloud API version to 0.13.0-beta * use package target * tested with FINE log level * Simplify entrypoint args processing The $@ arg array is kept complete between all scripts. * remove debug * remove debug * Test that the logging dependencies are hidden from webapp classpath * update README.md with instructions to keep INFO level on io.grpc.netty.level=INFO * Updated to lasted openjdk-runtime * fixed classloader test * fixed TODO * Update to jetty 9.4.5 and gcloud 1.0.1-SNAPSHOT * upgraded to gcloud-logging 1.0.1 * turn off debug * updated README for latest 1.0.1 gcloud-logging * update classpath exclusion \o/
Our team recently started migrating from GAE Standard to Flexible, and I bumped into this issue while investigating the difference in logging behaviour. My understanding is that the change done here will collapse multi-line logs into a single Stackdriver log entry, and will propagate JUL logging levels to Stackdriver. That will indeed make Flexible logging significantly more useful, but it still seems to be missing the feature we found extremely convenient in Standard: grouping Jetty logs by HTTP requests. Can I ask if this feature is still in the plans, and if not, why? We are considering doing that with a custom JUL logging handler that will buffer the logs, and flush the buffer at the end of the request or if the buffer grows too big, producing a Standard-style Stackdriver log entry of |
Other than collapsing multi-line log messages, and setting the levels correctly, in the #81 we have support for enhanced logging which will provide grouping by HTTP request. You just have to make sure you configure the |
Implement stackdriver logging as described in the jetty-runtime design document.
The text was updated successfully, but these errors were encountered: