-
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
fix: Regular error showing Total timeout of API google.logging.v2.LoggingServiceV2 exceeded 600000 milliseconds #1225
Conversation
…gingServiceV2 exceeded 600000 milliseconds
@@ -60,6 +60,7 @@ export interface LogOptions { | |||
removeCircular?: boolean; | |||
maxEntrySize?: number; // see: https://cloud.google.com/logging/quotas | |||
jsonFieldsToTruncate?: string[]; | |||
defaultWriteDeleteCallback?: ApiResponseCallback; |
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.
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.
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.
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"
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.
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?
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.
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; |
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.
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?
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." |
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 |
No region tags are edited in this PR.This comment is generated by snippet-bot.
|
Warning: This pull request is touching the following templated files:
|
from 9.0.0 to 9.6.9 Error "GoogleError: Total timeout of API google.logging.v2.LoggingServiceV2 exceeded 60000 milliseconds before any response was received" has been fixed in version9.6.9 of @google-cloud/logging: googleapis/nodejs-logging#1185 googleapis/nodejs-logging#1225
This is a proposed fix to accept a global callback for
Log.delete
andLog.write
methods throughLogOptions
Fixes #<1185> 🦕