-
Notifications
You must be signed in to change notification settings - Fork 99
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
Add option to parse error logs to Error Reporting type #667
Conversation
exporter/collector/logs_test.go
Outdated
{ | ||
Payload: map[string]interface{}{ | ||
"@type": "type.googleapis.com/google.devtools.clouderrorreporting.v1beta1.ReportedErrorEvent", | ||
"message": `{"message": "hello!"}`, |
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.
note that "json" bodies (that are actually string values) can end up with a redundant message key. I don't think we should try automatically parsing bodies that are string valued to JSON and making assumptions to remove them
exporter/collector/integrationtest/testdata/fixtures/logs/logs_apache_error.json
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #667 +/- ##
==========================================
+ Coverage 68.47% 68.56% +0.08%
==========================================
Files 36 36
Lines 4559 4577 +18
==========================================
+ Hits 3122 3138 +16
- Misses 1284 1286 +2
Partials 153 153
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Small nit (variable naming), but looks good to me overall.
@@ -180,6 +180,9 @@ type LogConfig struct { | |||
// service.name, service.namespace, and service.instance.id resource attributes into the Cloud Logging LogEntry labels. | |||
// Disabling this option does not prevent resource_filters from adding those labels. Default is true. | |||
ServiceResourceLabels bool `mapstructure:"service_resource_labels"` | |||
// ErrorReportingType enables automatically parsing error logs to a json payload containing the | |||
// type value for GCP Error Reporting. See https://cloud.google.com/error-reporting/docs/formatting-error-messages#log-text. | |||
ErrorReportingType bool `mapstructure:"error_reporting_type"` |
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.
nit: it isn't clear from the name what it does. Prefer send_errors_to_error_reporting, or something that implies what the effect is.
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.
afaict, adding this attribute doesn't send them "to" error reporting. they're still sent to cloud logging, just reformatted to have a special attribute that Error Reporting picks up. I'm not entirely clear on the ux from that point, will talk to some of the folks who work on ER
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.
Sounds good. Thanks for checking!
Fixes #665
This adds an option to the logs exporter,
log.error_reporting_type
, to automatically format error logs to GCP Error Reporting. To do this, it adds the@type
field to existing JSON payloads, or converts string payloads to JSON (with amessage
field) and then adds@type
.I don't think there is a behavior that makes sense for byte payloads, since that would mean a mixture of byte and string values in the json payload (?).
An improvement to this could be using the
ReportedErrorEvent
protobuf directly rather than just setting@type
, as documented in https://cloud.google.com/error-reporting/docs/formatting-error-messages#reported-error-example. This adds a requirement for aservice
field.