-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[libbeat] Stop publisher properly #40572
Conversation
c75d9f9
to
6b97469
Compare
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
looks good, but I'll wait the tests to pass to approve it |
r, err := http.Get("http://localhost:5066") //nolint:noctx // fine for tests | ||
require.NoError(t, err) | ||
require.Equal(t, http.StatusOK, r.StatusCode, "incorrect status code") |
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.
[Blocker] cont.
you could use it instead
r, err := http.Get("http://localhost:5066") //nolint:noctx // fine for tests | |
require.NoError(t, err) | |
require.Equal(t, http.StatusOK, r.StatusCode, "incorrect status code") | |
buff := &bytes.Buffer{} | |
require.Eventually(t, func() bool { | |
buff.Reset() | |
r, err := http.Get("http://localhost:5066") | |
if err != nil { | |
_, _ = fmt.Fprintf(buff, "stats endpoint not available: %v", err) | |
return false | |
} | |
if r.StatusCode != http.StatusOK { | |
_, _ = fmt.Fprintf(buff, "stats endpoint: bad HTTPnstatus: %s", | |
r.Status) | |
return false | |
} | |
return true | |
}, time.Second, 100*time.Millisecond, | |
"stats endpoint never become available: %s", buff) |
if you want, you could even remove the WaitForLogs
. Just don't mix WaitForLogs
and Eventually
because WaitForLogs
uses eventually underneath.
@@ -57,12 +57,14 @@ output.console: | |||
mockbeat.WriteConfigFile(cfg) | |||
mockbeat.Start() | |||
mockbeat.WaitForLogs("Starting stats endpoint", 60*time.Second) | |||
time.Sleep(time.Second) |
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.
[Blocker]
I see why just WaitForLogs("Starting stats endpoint"
isn't enough:
Lines 69 to 72 in 9249566
s.log.Info("Starting stats endpoint") | |
go func(l net.Listener) { | |
s.log.Infof("Metrics endpoint listening on: %s (configured: %s)", l.Addr().String(), s.config.Host) | |
err := http.Serve(l, s.mux) |
but we don't use time.Sleep
anymore, unless there is no other way. However here there is a better alternative. See below
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.
to avoid blocking the PR and it not making to 8.15.1, I'll approve and later open another PR to properly fix those tests. The issue is on libbeat, the mock beat does not stops properly, if it did, your PR would not have impacted those tests
* Stop publisher properly * Just call beater.Stop from manager * Delete duplicated lines * Make call to stopBeat idempotent * Add context at request creation to not break tracing * Remove unused lint * Add default WaitClose timeout * Adjust wait on close time * Add delay to account for the stop of the publisher * Fix lint issues * Fix lint issues * Fix lint (cherry picked from commit 4808269)
* Stop publisher properly * Just call beater.Stop from manager * Delete duplicated lines * Make call to stopBeat idempotent * Add context at request creation to not break tracing * Remove unused lint * Add default WaitClose timeout * Adjust wait on close time * Add delay to account for the stop of the publisher * Fix lint issues * Fix lint issues * Fix lint (cherry picked from commit 4808269) Co-authored-by: Marc Guasch <[email protected]>
Proposed commit message
We avoided closing the publisher on Stop to avoid races with the ES output since we were not aborting the active connections. This adds:
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Disruptive User Impact
Related issues