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

fix: Regular error showing Total timeout of API google.logging.v2.LoggingServiceV2 exceeded 600000 milliseconds #1225

Merged
merged 22 commits into from
Feb 9, 2022

Conversation

losalex
Copy link
Contributor

@losalex losalex commented Jan 31, 2022

This is a proposed fix to accept a global callback for Log.delete and Log.write methods through LogOptions

Fixes #<1185> 🦕

@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: logging Issues related to the googleapis/nodejs-logging API. labels Jan 31, 2022
@losalex losalex requested a review from minherz January 31, 2022 23:03
@@ -60,6 +60,7 @@ export interface LogOptions {
removeCircular?: boolean;
maxEntrySize?: number; // see: https://cloud.google.com/logging/quotas
jsonFieldsToTruncate?: string[];
defaultWriteDeleteCallback?: ApiResponseCallback;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this global callback only for WriteDelete? I thought you want to have a global callback for all methods in Log class in manual layer.

Copy link
Contributor Author

@losalex losalex Feb 1, 2022

Choose a reason for hiding this comment

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

It is for write() and delete() APIs which are the only one's used to update logs. Just wanted to distinguish it to make sure we do not confuse developers. Perhaps naming WriteDelete is confusing? I can make it defaultWriteCallback to emphasize the "writing"

Copy link
Contributor

Choose a reason for hiding this comment

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

a small correction:delete() does not update logs, it deletes all log entries for a provided log name as a result of the delete log operation while write() creates one or more log entries.

i am not sure about the "global" scope. the scope is bound by the Log instance, isn't it?

Copy link
Contributor

@minherz minherz left a comment

Choose a reason for hiding this comment

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

From the issue (#1185) it is not clear what developers except to get. The complains sound like they do not want an application to crash in a case the retries fail to ingest the log.
In this respect, the fix does not resolve the problem if the callback is not provided.

@@ -60,6 +60,7 @@ export interface LogOptions {
removeCircular?: boolean;
maxEntrySize?: number; // see: https://cloud.google.com/logging/quotas
jsonFieldsToTruncate?: string[];
defaultWriteDeleteCallback?: ApiResponseCallback;
Copy link
Contributor

Choose a reason for hiding this comment

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

a small correction:delete() does not update logs, it deletes all log entries for a provided log name as a result of the delete log operation while write() creates one or more log entries.

i am not sure about the "global" scope. the scope is bound by the Log instance, isn't it?

@losalex
Copy link
Contributor Author

losalex commented Feb 1, 2022

From the issue (#1185) it is not clear what developers except to get. The complains sound like they do not want an application to crash in a case the retries fail to ingest the log. In this respect, the fix does not resolve the problem if the callback is not provided.

Based on user's feedback in a bug, the suggestion was: "I assume a lot of users will want to set up some sort of global error catching for this particular error."
Do you have specific proposal in mind how to fix this?

@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Feb 2, 2022
@losalex losalex marked this pull request as ready for review February 2, 2022 17:29
@losalex losalex requested review from a team as code owners February 2, 2022 17:29
@minherz
Copy link
Contributor

minherz commented Feb 2, 2022

The quoted suggestion comes from one out of four people who describe scenarios that generally caused by failure in retrying mechanism due to transient connectivity error or because the ran their application in the not connected environment.

One of the cases resulted in printing the error to STDOUT where it was catched by a logging agent and eventually reported to Cloud Logging backend.

In my opinion the problem is when this error crashes the application. It is a good idea to provide a way to define the error callback. I suggest to add a default error handling if nothing is provided, so the application would not crash.

@losalex
Copy link
Contributor Author

losalex commented Feb 2, 2022

The quoted suggestion comes from one out of four people who describe scenarios that generally caused by failure in retrying mechanism due to transient connectivity error or because the ran their application in the not connected environment.

One of the cases resulted in printing the error to STDOUT where it was catched by a logging agent and eventually reported to Cloud Logging backend.

In my opinion the problem is when this error crashes the application. It is a good idea to provide a way to define the error callback. I suggest to add a default error handling if nothing is provided, so the application would not crash.

I believe we cannot provide default callback since client may call log.write with await and put the call inside try/catch - this way default callback from code will "swallow" any thrown error, thus catch becomes to be redundant. Worth mentioning that it is eventually will violate our contract with users who does await with try/catch.

@snippet-bot
Copy link

snippet-bot bot commented Feb 3, 2022

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@generated-files-bot
Copy link

Warning: This pull request is touching the following templated files:

.readme-partials.yml Outdated Show resolved Hide resolved
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 googleapis/nodejs-logging API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants