Skip to content

partial success on event-exporter writing logs#475

Open
snowhork wants to merge 1 commit intoGoogleCloudPlatform:masterfrom
snowhork:partial-success-on-writer-log
Open

partial success on event-exporter writing logs#475
snowhork wants to merge 1 commit intoGoogleCloudPlatform:masterfrom
snowhork:partial-success-on-writer-log

Conversation

@snowhork
Copy link

@snowhork snowhork commented Jun 4, 2022

We should enable partial success on writing log entries.

In the for loop, some entries are in bad format, and it receives StatusBadRequest. But, if partials success is not enabled, some appropriate entries may lose.

@google-cla
Copy link

google-cla bot commented Jun 4, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@snowhork snowhork force-pushed the partial-success-on-writer-log branch from 26f5a3f to 4fadf2f Compare June 4, 2022 14:24
@snowhork
Copy link
Author

snowhork commented Jun 4, 2022

@osalau @sophieliu15 please review.

Entries: entries,
LogName: logName,
Resource: resource,
PartialSuccess: true,
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for sending in the PR!

We also need to count how many entries are successfully delivered in our metrics here and their corresponding latency: https://github.com/GoogleCloudPlatform/k8s-stackdriver/blob/master/event-exporter/sinks/stackdriver/writer.go#L66-L67

If there is an easy way to count those, could you update corresponding metrics? If not, feel free to send us a feature request PR and we will prioritize the work based on our team capacity and priority.

Copy link
Author

Choose a reason for hiding this comment

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

@sophieliu15 Thanks for your reply! I will try it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants