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

feat: use standard output logs on serverless environments #228

Merged
merged 23 commits into from Apr 9, 2021

Conversation

daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Mar 18, 2021

We have had bug reports that some logs aren't getting through when using the library in serverless environments, because the environment will spin down before logs are flushed.

To fix this, we can log to standard output in JSON format, and allow the environment's native log handling code to extract the logs to Stackdriver.

This method is already used for GKE. Instead of reusing the ContainerEngineHandler, we will create a new more general StructuredLogHandler, and deprecate ContainerEngineHandler at the next major relase

Merging this branch into v2_update_2 to group work for current milestone.

#226

@daniel-sanche daniel-sanche requested review from as code owners Mar 18, 2021
@product-auto-label product-auto-label bot added the api: logging label Mar 18, 2021
@google-cla google-cla bot added the cla: yes label Mar 18, 2021
@daniel-sanche daniel-sanche changed the title [DRAFT] use standard output logs on serverless environments feat: use standard output logs on serverless environments Mar 18, 2021
@daniel-sanche daniel-sanche changed the base branch from master to v2_update_2 Apr 1, 2021
# make logs appear in GCP structured logging format
self.formatter = logging.Formatter(GCP_FORMAT)

def format(self, record):

Choose a reason for hiding this comment

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

don't you need to check that record is not null?

Copy link
Contributor Author

@daniel-sanche daniel-sanche Apr 2, 2021

Choose a reason for hiding this comment

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

That shouldn't be necessary. We're just delegating the formatting work to another standard Python formatter object. If there were any error cases to worry about, the function we're calling can handle it.

if monitored_resource.type == _GAE_RESOURCE_TYPE:
return AppEngineHandler(self, **kw)
elif monitored_resource.type == _GKE_RESOURCE_TYPE:
return ContainerEngineHandler(**kw)

Choose a reason for hiding this comment

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

how is ContainerEngineHandler different from StructuredLogHandler?

Copy link
Contributor Author

@daniel-sanche daniel-sanche Apr 2, 2021

Choose a reason for hiding this comment

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

They are essentially the same, but I'm going to leave ContainerEngineHandler in for now for API consistency. I plan on removing ContainerEngineHandler and AppEngineHandler in v3.0.0 in favor of more general classes

google/cloud/logging_v2/client.py Outdated Show resolved Hide resolved
"AppEngineHandler",
"CloudLoggingHandler",
"ContainerEngineHandler",
"StructuredLogHandler",
"setup_logging",

Choose a reason for hiding this comment

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

Worth adding a comment to explain semantics of functionality that each handler adds.

Copy link
Contributor Author

@daniel-sanche daniel-sanche Apr 2, 2021

Choose a reason for hiding this comment

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

I don't believe it's standard to use __init__.py for documentation.This is just an internal file used for importing packages. It's likely to get out of date if things change in the future, so I'd prefer to keep documentation alongside code

record.trace = getattr(record, "trace", inferred_trace) or ""
record.http_request = getattr(record, "http_request", inferred_http) or {}
record.request_method = record.http_request.get("requestMethod", "")
record.request_url = record.http_request.get("requestUrl", "")
record.user_agent = record.http_request.get("userAgent", "")
record.protocol = record.http_request.get("protocol", "")

Choose a reason for hiding this comment

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

are you planning to do anything about log severity too?

Copy link
Contributor Author

@daniel-sanche daniel-sanche Apr 2, 2021

Choose a reason for hiding this comment

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

This "Filter" class is to add new fields to the LogRecord object, that we can then use later when exporting logs in the Handler classes. Common fields like "severity" already exist in the LogRecord by default, so no need to add them here

Copy link
Contributor

@0xSage 0xSage Apr 9, 2021

Choose a reason for hiding this comment

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

are you planning to add other httprequest fields from here as well. Nonblocking comment.

Copy link
Contributor Author

@daniel-sanche daniel-sanche Apr 9, 2021

Choose a reason for hiding this comment

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

eventually, but some of them are more complicated, and I want to make sure they work consistently across environments. I want to put in the basics first



class StructuredLogHandler(logging.StreamHandler):
"""Handler to format logs into the Cloud Logging structured log format,
Copy link
Contributor

@0xSage 0xSage Apr 9, 2021

Choose a reason for hiding this comment

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

Maybe add a link to https://cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry#LogSeverity or another appropriate link, so they know "structured log format" is a formally defined hting

0xSage
0xSage approved these changes Apr 9, 2021
Copy link
Contributor

@0xSage 0xSage left a comment

Worth adding samples for this. Also to more thoroughly document this in python-logging docs.

@daniel-sanche
Copy link
Contributor Author

@daniel-sanche daniel-sanche commented Apr 9, 2021

Worth adding samples for this. Also to more thoroughly document this in python-logging docs.

I have an open bug for re-doing the samples/usage guide in v3.0.0. I expect more changes before then

@daniel-sanche daniel-sanche merged commit a78f577 into v2_update_2 Apr 9, 2021
13 checks passed
@daniel-sanche daniel-sanche deleted the structured-log-handler-2 branch Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants