Additional logs for the grpc conn. #1319
Conversation
huseyinbabal
requested review from
a team,
madebyrogal and
pkosiec
and removed request for
madebyrogal
There was a problem hiding this comment.
Not sure if the timeout isn't too small, but let's. test the PR image on our Botkube environments to see if that helps for a longer period of time 👍
Also, consider also adding https://grpc.io/docs/guides/deadlines/ to the server-side too if that makes sense 👍
|
As a summary;
|
| if !lastFailureTimestamp.IsZero() && time.Since(lastFailureTimestamp) >= successIntervalDuration { | ||
| // if the last run was long enough, we treat is as success, so we reset failures | ||
| log.Infof("Resetting failures counter as last failure was more than %s ago", successIntervalDuration) | ||
| b.failuresNo = 0 |
There was a problem hiding this comment.
we can also reset the failure msg here, or it doesn't make sense?
| }, | ||
| retry.OnRetry(func(_ uint, err error) { | ||
| log.Warnf("Retrying Cloud Teams startup (attempt no %d/%d): %s", b.failuresNo, maxRetries, err) | ||
| }), | ||
| retry.Delay(retryDelay), | ||
| retry.DelayType(resettableBackoff), | ||
| retry.Attempts(0), // infinite, we cancel that by our own | ||
| retry.LastErrorOnly(true), | ||
| retry.Context(ctx), |
There was a problem hiding this comment.
should we also add something similar here to
b.failuresNo = 0 // Reset the failures to start exponential back-off from the beginning
b.setFailureReason("")
b.log.Info("Botkube connected to Slack!")?
There was a problem hiding this comment.
I couldn't get the point here, could you please elaborate a bit more? My bad, yes now I get it, I will modify teams
There was a problem hiding this comment.
while it wasnt the actual reason, imho it would be good to add the same what we have in cloud teams:
ctxTimeout, cancelFn := context.WithTimeout(ctx, cloudTeamsConnectTimeout)
defer cancelFn()
err = svc.Start(ctxTimeout)
if err != nil {
return fmt.Errorf("while starting gRPC connector %w", err)
}There was a problem hiding this comment.
Actually, I should remove this from teams too, I was testing slack first. So the problem with ctxTimeout, it cancels the existing streaming connection event after the start is initiated. Assume, I provided 15 secs timeout, connection is created successfully, but during the streaming it just cancelled since it is not only used in connection, it is also used in grpc operation.
huseyinbabal
force-pushed
the
platform-conn-check
branch
from
0a42954 to
e3545a3
Compare
Description
Changes proposed in this pull request:
Testing
Related issue(s)