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

Missing Documentation for per-request Cloud Logging in Flex. #720

Closed
danjacques opened this issue Aug 7, 2017 · 14 comments
Closed

Missing Documentation for per-request Cloud Logging in Flex. #720

danjacques opened this issue Aug 7, 2017 · 14 comments
Assignees
Labels
api: logging Issues related to the Cloud Logging API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. 🚨 This issue needs some love. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@danjacques
Copy link

I'm converting an AppEngine classic environment service over to AppEngine Flex environment. One of the services that I'm trying to port is logging. In Classic, one uses the logger as follows:

import "google.golang.org/appengine/log"
log.Infof(ctx, "Doing something.")
log.Errorf(ctx, "Something went wrong!")

When viewing AppEngine's logs, these logs will be grouped under a request, like:

HTTP 200 /
\- (I) Doing something.
\- (E) Something went wrong!

In Flex environment, however, there are multiple ways to log. The simplest is to use os.Stderr to log to STDERR; however, this just shows up as a series of top-level logs, not related by HTTP request. One is better off using the Cloud Logging package. However, I can't seem to find the right combination of settings to cause logs emitted with this package to group by request.

The Logger is very configurable, and documentation in the package suggests that leaving fields as default will automatically detect and populate them, and looking at the logging UI, this seems true to some extent. However, the default configuration in a Flex environment seems to still not group logs by request.

I stumbled upon an issue that suggests that it is possible, and that populating the HTTPRequest field and/or the appengine.googleapis.com/trace_id label will accomplish this, but it doesn't seem to be working for me.

With respect to this repository, I'd like to request an example be included in the Go documentation detailing how to setup a logger instance that will operate in a Flex environment in a manner similar to AppEngine Classic. Specifically, how should one populate:

@jba
Copy link
Contributor

jba commented Aug 7, 2017

Can you try this:

  1. The "parent" log entry must have httpRequest.requestUrl populated (i.e., it should be a HTTP request-style entry)
  2. The "child" log entry must be in a different log stream (logName) than the parent, in the same project, and have the exact same resource.type and resource.labels values
  3. The child entry's timestamp must be within the time interval covered by the parent request (i.e., older than parent.timestamp, and newer than parent.timestamp - parent.httpRequest.latency) -- this assumes that the timestamp of the "request" entry is the end time, not the start time
  4. The trace field must be populated in all of the entries and match exactly

Please let us know whether that works or doesn't.

@danjacques
Copy link
Author

Thanks for the info! After some tweaking, I have gotten HTTP request logs to group together:

client, _ := logging.NewClient(context.Background(), probedGCEProjectID)
logger := client.Logger(
	"stderr",
	logging.CommonLabels(map[string]string{
		"appengine.googleapis.com/trace_id": extractTraceIDFromHeader(req),
	}),
	logging.CommonResource(&mrpb.MonitoredResource{
		Labels: map[string]string{
			"module_id":  probedGCEServiceName,
			"project_id": probedGCEProjectID,
			"version_id": probedGCEVersionName,
		},
		Type: "gae_app",
	}),
)

This is all very manual. The default MonitoredResource uses different labels ("project_id", "instance_id", "zone") and type ("gce_instance"). The trace ID has to be parsed from the X-Cloud-Trace-Context HTTP header. Am I missing a helper library, or is this WAI?

I also note that the overall request's severity icon, which is usually the highest child log severity, is not being updated based on child contents. Is there some additional information that I can add to make this work?

@jba
Copy link
Contributor

jba commented Aug 7, 2017

Glad you got it to work (mostly). I've contacted the log team internally about the severity.

/cc @rakyll for propagating trace.
/cc @pongad @zombiezen @adams-sarah for whether it makes sense to simplify this.

@jba jba self-assigned this Aug 8, 2017
@jba jba added documentation api: logging Issues related to the Cloud Logging API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. labels Aug 8, 2017
@danjacques
Copy link
Author

Any update on this? I'd like to solve this if possible, since the lack of top-level messages is a regression from my Managed VM code. An example of what I'm experiencing is:
Screenshot of application logs

The code that sets up logging is here if that is useful.

Filtering logs by minimum level also doesn't seem to be working, for what I'm assuming right now is the same reason. Thoughts?

@zombiezen
Copy link
Contributor

@danjacques: Thanks for the feedback. We're aware that this is a painpoint and we're looking into how to simplify this common case. The difficulty in creating a simple API is that we want stderr to be usable (since many things log there and it's the easiest thing that shows up in GKE) and we don't want to duplicate log messages (logging to stderr and to Stackdriver with structured severity would duplicate).

In the nearer term, we know that monitored resource auto-detection could be better. Subscribe to #650 for updates.

@danjacques
Copy link
Author

Agreed on API, and it would be interesting to see where you go with that.

WRT nearer term and resource detection - I'm 100% fine with manually entering the resource fields. Is there a field that I'm missing that would cause the top-level log entries to display and be recognized at the level of their highest sub-log? Is there an example of a application that uses cloud logging and achieves this effect?

The way I see things, I'm either not doing this properly, or this feature is not supported at the moment. Can you clarify which is the case?

@zombiezen
Copy link
Contributor

We would have to do more investigation to determine which of those is the case. AFAIK right now, you are using the API properly. This may be a logs UI issue.

@danjacques
Copy link
Author

OK cool, thanks for the information. Please let me know if there's anything that I can do to help test!

ATM this is a non-blocking but pretty obvious regression from the GAE environment. I'm migrating from Managed VM (deprecated) to Flex (new hotness) and this specifically stands out as a shortcoming.

@anthmgoogle anthmgoogle added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Nov 10, 2017
@vadimsht
Copy link

Note that constructing Logger per request (to have trace_id in CommonLabels) leads to a rapid goroutine (and memory) leaks, since Client "remembers" all Loggers until it itself is closed. In particular, goroutines get blocked on https://github.com/GoogleCloudPlatform/google-cloud-go/blob/d30a909c68aade42fadac37751a1ba349dac77e4/logging/logging.go#L398

So either requests must reuse Loggers, or they must not share Clients. (Or it's a bug and it should be fixed...).

chromium-infra-bot pushed a commit to luci/gae that referenced this issue Nov 22, 2017
Otherwise we leak Logger, since they are not released until Client is closed
(which is never).

See googleapis/google-cloud-go#720 (comment)

R=iannucci@chromium.org
BUG=783023

Change-Id: I9ecdc16bd59e49d347ac846804f2a14d6fb82d10
Reviewed-on: https://chromium-review.googlesource.com/783843
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Vadim Shtayura <vadimsh@chromium.org>
@derekperkins
Copy link
Contributor

@danjacques I got logging working on Flex pretty much the same way it used to work. My implementation details are here: #448 (comment)

@JustinBeckwith JustinBeckwith added the 🚨 This issue needs some love. label Jun 8, 2018
@jba
Copy link
Contributor

jba commented Jul 26, 2018

As of 7f7e4d3 the logging client automatically sets Entry.Trace to the value of the X-Cloud-Trace-Context header if it isn't already set.

@jba
Copy link
Contributor

jba commented Aug 2, 2018

Please let me know if https://code-review.googlesource.com/c/gocloud/+/31310 explains this clearly.

With that addition to the docs, I'm going to close this issue. I don't think there's anything more to be done with this client.

@derekperkins
Copy link
Contributor

Parent entries must have HTTPRequest.Request populated

@jba Is that strictly necessary? We're moving from a request driven task queue to a long-running worker system, and I would still like to group each individual unit of work by a unique trace id. Is that not possible without creating an http request object?

@jba
Copy link
Contributor

jba commented Aug 3, 2018

@derekperkins You do need the request URL to enable grouping, but not the whole request, and the URL doesn't have to be real:

parentLogger.Log(logging.Entry{
        HTTPRequest: &logging.HTTPRequest{Request: &http.Request{
            URL: &url.URL{Path: "dummy/path"},
        }},
        Payload: "whatever",
        Trace:   traceID,
    })

Non-request log entries with the same trace ID will group with this one. The text payload won't be visible on the log line, but is visible once you click the entry to open it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. 🚨 This issue needs some love. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

7 participants