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

Log reconnect attempts #6404

Merged
merged 2 commits into from
Aug 6, 2018
Merged

Log reconnect attempts #6404

merged 2 commits into from
Aug 6, 2018

Conversation

recrsn
Copy link
Contributor

@recrsn recrsn commented Feb 18, 2018

Closes #5175

Logs an info line "Attempting to reconnect" at every reconnect attempt.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@kvch
Copy link
Contributor

kvch commented Feb 18, 2018

jenkins test it

@@ -69,12 +71,18 @@ func (w *netClientWorker) run() {
return
}

if attemptReconnect {
logp.Info("Attempting to reconnect.")
}
Copy link

@urso urso Feb 20, 2018

Choose a reason for hiding this comment

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

+1 for logging connection and reconnect attempts

We have two loops in this methods. First loop attempts to connect. Second loop processes events.

The first loop becomes active after beats startup, once the first event is to be published. That is connecting the outputs is done lazily. In this case the log message should indicate this being the initial connect.

Once the send loop fails, we're about to reconnect.

Having a counter on failed connect attempts would be nice to have as well.

E.g. for the lifetime of an output it would be nice to have log messages like (assuming we would have an 'identifier' per output):

Startup output to <identifier>
Connect to <identifier>
Failed to connect to <identifier>: <reason>
Connect to <identifier>
Connection to <identifier> established
Failed to publish events to <identifier>: %v
Closed connection to <identifier>
Attempting to reconnect to <identifier> with 0 reconnect attempt(s).
Failed to connect to <identifier>: <reason>
Attempting to reconnect to <identifier> with 1 reconnect attempt(s).
Connection to <identifier> established
Closed connection to <identifier>
Shutdown output to <identifier>

That is, we have 4 phases/states to distinguish:

  1. init worker
  2. create initial connection
  3. reconnect
  4. shutdown

Having an enum to differentiate on state and logging should help here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to use hostname as an identifier. However, we don't have it here.
I'll be adding additional logging lines for other events.

@recrsn
Copy link
Contributor Author

recrsn commented Feb 24, 2018

@urso We counts for reconnect attempts now.

@recrsn
Copy link
Contributor Author

recrsn commented Feb 24, 2018

@urso I need some pointers for how to create the identifier.

The state enum should probably go into the Connection type ?

@ruflin
Copy link
Member

ruflin commented Feb 27, 2018

@agathver The team is at Elastic{ON} / All hands this and next week so it will take some time to respond.

@recrsn
Copy link
Contributor Author

recrsn commented Mar 12, 2018

Any updates on this?

@urso
Copy link

urso commented Mar 13, 2018

The counting looks fine.

It would be nice to have an identifier in general. It's the client knowing about the actual endpoint/ID in use. Let's add the identifier to the output.Client.

type Client interface {
        String() string // alternative names: ID(), Name(), Endpoint(), Target()
	Close() error
	Publish(publisher.Batch) error
}

I used the String() method, as this way the client can be pushed as parameter to the logging functions.

The console output will return "stdout", the file output the path and the network based outputs the url/host/port.

@recrsn
Copy link
Contributor Author

recrsn commented Mar 15, 2018

@urso, Added identifiers for connections.

I have used a composition-like approach for naming (borrowed from the convention in ReactJS), in tune with how most clients are being used.

Example, a redis client wrapped with a failover will be failover(redis://10.0.0.1, redis://10.0.0.2)
Default ES config gives backoff(elasticsearch(http://localhost:9200))

@recrsn recrsn force-pushed the reconnect-logging branch 2 times, most recently from b455c64 to 178cac5 Compare April 6, 2018 15:44
Copy link

@urso urso left a comment

Choose a reason for hiding this comment

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

Great improvement. Sorry for having you wait. Thank you for taking the time to add these!

@urso urso merged commit 06dea8b into elastic:master Aug 6, 2018
@recrsn recrsn deleted the reconnect-logging branch August 9, 2018 12:17
@urso urso added the needs_backport PR is waiting to be backported to other branches. label Aug 16, 2018
@urso urso added v6.5.0 and removed needs_backport PR is waiting to be backported to other branches. labels Aug 29, 2018
urso pushed a commit to urso/beats that referenced this pull request Aug 29, 2018
* Log reconnect attempts (elastic#5715)

* Add identifiers to connections

(cherry picked from commit 06dea8b)
@urso urso added the v6.4.1 label Aug 29, 2018
ph pushed a commit that referenced this pull request Sep 4, 2018
Cherry-pick of PR #6404 to 6.4 branch. Original message: 

Closes #5175 

Logs an info line "Attempting to reconnect" at every reconnect attempt.
urso pushed a commit to urso/beats that referenced this pull request Oct 19, 2018
* Log reconnect attempts (elastic#5715)

* Add identifiers to connections

(cherry picked from commit 06dea8b)
urso pushed a commit that referenced this pull request Oct 22, 2018
* Log reconnect attempts (#6404)

* Log reconnect attempts (#5715)

* Add identifiers to connections

(cherry picked from commit 06dea8b)

* Add String() to redis backoff client to correctly impl. NetworkClient (#7882)

(cherry picked from commit 551b2a9)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
Cherry-pick of PR elastic#6404 to 6.4 branch. Original message: 

Closes elastic#5175 

Logs an info line "Attempting to reconnect" at every reconnect attempt.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants