-
Notifications
You must be signed in to change notification settings - Fork 537
fix: remove hardcoded timeout and don't block when fetching sourcemaps #10367
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: remove hardcoded timeout and don't block when fetching sourcemaps #10367
Conversation
|
This pull request does not have a backport label. Could you fix it @kruskall? 🙏
NOTE: |
📚 Go benchmark reportDiff with the report generated with https://pkg.go.dev/golang.org/x/perf/cmd/benchstat |
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.
Thanks, this is looking better.
We should pretty much never have a time.Sleep(...) in a test -- that will lead to flakiness. So if that wasn't just for debugging, let's find a deterministic alternative please.
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.
Thanks, almost there. Just needs a change in the tests, otherwise looks good.
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.
LGTM, thanks
#10367) * test: disable kibana fallback to avoid false positives * fix: fall back immediately if the metadata fetcher is not ready * test: add back delete hook for sourcemaps using the kibana api * refactor: remove arbitrary 500ms timeout in chained fetcher * test: disable kibana in nomatching sourcemap test * test: update approvals results * test: add time sleep to wait for init * refactor: avoid time sleep in tests * refactor: remove es ping and rely on initial sync request * refactor: remove arbitrary 10s timeout and rely on es client timeout * lint: fix linter issues * test: refactor es config test and remove wait method from interface * lint: remove unused methods (cherry picked from commit 34d3395) # Conflicts: # systemtest/kibana.go
…rcemaps (backport #10367) (#10399) * fix: remove hardcoded timeout and don't block when fetching sourcemaps (#10367) * test: disable kibana fallback to avoid false positives * fix: fall back immediately if the metadata fetcher is not ready * test: add back delete hook for sourcemaps using the kibana api * refactor: remove arbitrary 500ms timeout in chained fetcher * test: disable kibana in nomatching sourcemap test * test: update approvals results * test: add time sleep to wait for init * refactor: avoid time sleep in tests * refactor: remove es ping and rely on initial sync request * refactor: remove arbitrary 10s timeout and rely on es client timeout * lint: fix linter issues * test: refactor es config test and remove wait method from interface * lint: remove unused methods (cherry picked from commit 34d3395) # Conflicts: # systemtest/kibana.go * fix: resolve conflicts --------- Co-authored-by: kruskall <[email protected]>
Motivation/summary
A couple of changes:
TestNoMatchingSourcemapswas failing because the resulting document had a sourcemap applied. It looks like there was a sourcemap in ES despite thecleanupElasticsearchcall. Reverting the delete hook seems to make things more deterministic.Checklist
apmpackagehave been made)For functional changes, consider:
How to test these changes
Related issues
Related to #10338