Improve GoogleCloudConsoleFormatter to support log based GCP error reporting

[danielwinkler]

Currently the error reporting is only picking up from logs if the log has a stack trace, as described in the docs.

However, I would like to have the possibility to get also informed when logs reach a certain severity, e.g. Error.

The idea of the PR #14257 is to be able to configure a ErrorReportingLogLevel, and logs higher or equal will get annotated in the structured logging such that it will trigger the error reporting.

Additionally, the PR allows to add a service context, by specifying ServiceIdentifier and ServiceVersion.
This in turn will improve the ability to filter the errors in the "Error reporting".

[amanda-tarafa]

Thanks for creating the issue. I'll confirm a couple of things internally and get back to you.

[danielwinkler]

Thank you very much @amanda-tarafa

[amanda-tarafa]

I've looked at this some more and I have the following questions/observations/opinions. Please call out if I'm missing something.

• I don't see why we need to add a separate stack_trace field when we already have and exception field, which, according to Error formating documentation is also used by Error Reporting to identify the entry as an error entry. Is this not working as expected?
• Setting a value for the service context options means that Error Reporting will identify as errors absolutely all the entries that pass through this formatter. I don't think that's a good idea, and if we were to accept it, it'd need to be way better documented than what it is on your PR. Quoting: "If you set the @type field to a different value or leave it unset, then Cloud Logging searches for field labeled serviceContext to determine if the payload is a ReportedErrorEvent object."
• Although I think it's reasonable to want log entries with error severity to be recognized as such by Error Reporting, regardles of what their payload is, I'm less sure about this package being involved in that. That seems more like something Error Reporting ingestion could already do. I will ask the Error Reporting team if there's any reason why they don't take the severity level into account already.

More broadly, this a general purpose Google Cloud Logging formatter, not an Error Reporting specific formatter, and in my opinion, its implementation shouldn't be too strongly tied to the Error Reporting heuristics for identifying error entries.

What would you think of an approach where we add an option Action<LogEntry, Utf8JsonWriter> PayloadCustomizer that you can set to a function that adds custom fields to the entry, including ErrorReporting specific fields?

[danielwinkler]

Hi @amanda-tarafa, thank you for your feedback.

I don't see why we need to add a separate stack_trace field when we already have and exception field, which, according to Error formating documentation is also used by Error Reporting to identify the entry as an error entry. Is this not working as expected?

you're right about this, this would also invalidate backwards compatibility of the formatter, so this should be dropped.
I wrote a custom formatter for my use case and copied this over by mistake.

Setting a value for the service context options means that Error Reporting will identify as errors absolutely all the entries that pass through this formatter. I don't think that's a good idea, and if we were to accept it, it'd need to be way better documented than what it is on your PR. Quoting: "If you set the @type field to a different value or leave it unset, then Cloud Logging searches for field labeled serviceContext to determine if the payload is a ReportedErrorEvent object."

This is not correct, i tested setting the service context on all logs and they are not getting picked up as errors.
So if you add it, it will have no effect on the error reporting, but still allows you to control the part of the log that may be useful for searching on the log only side.

Although I think it's reasonable to want log entries with error severity to be recognized as such by Error Reporting, regardles of what their payload is, I'm less sure about this package being involved in that. That seems more like something Error Reporting ingestion could already do. I will ask the Error Reporting team if there's any reason why they don't take the severity level into account already.

I agree with you that they could already pick up on it, although the instructions don't have any conditions on the log severity, which is in line with their behavior.
On the other hand you are giving your customers instructions on how they have to format the logs in order to be picked up by error reporting.
In that respect it would be useful if the tool that you provide your customers for logging supports this kind of formatting.

What would you think of an approach where we add an option Action<LogEntry, Utf8JsonWriter> PayloadCustomizer that you can set to a function that adds custom fields to the entry, including ErrorReporting specific fields?

I would be fine with this, but i would prefer if the client library would make it easy to configure the observability of GCP as i see logging and error reporting parts of this bigger concept.

Please let me know what you're thinking.

[amanda-tarafa]

Thanks for your replies. We are discussing internally and with the Error Reporting team what's the best way forward. You'll hear from us next week.

[danielwinkler]

Perfect, thanks so much.

[amanda-tarafa]

@danielwinkler We've discussed internally and with the Error Reporting team. Adding special fields to log entries is not just about those entries being recognized and displayed as errors by Error Reporting, it is also about grouping those entries together as occurrences of the same error. Different fields are used together or not to group entries, depending on the context, and not all of the heuristics are publicly documented, and that's intentional.

In a general purpose formatter such as this one, we feel more comfortable exposing an entry augmentation mechanism, for users to specify the combination of Error Reporting specific fields that suits their use case better, without our logger being tied to one or the other combination.

I'm happy to review a full PR for that, including tests, etc. if you want to have a go at it, else, we'll work on it ourselves in the next couple of weeks.

[danielwinkler]

@amanda-tarafa thanks for feedback, I created an implementation of your suggestion (#14284)

[amanda-tarafa]

Thanks @danielwinkler , I'll take a look as soon as I can, but it might well be early next week.