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

Use different timeout values for CPU and non-CPU diagnostic requests #3794

Merged
merged 3 commits into from
Nov 27, 2023

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Nov 20, 2023

What does this PR do?

Part of #3197, this is a fairly simple change that has the diagnostic request use two different timeout values depending on if we've requested additional CPU metrics. This isn't meant to solve #3197, but serves to fix one of the causes.

Why is it important?

This is meant to prevent us from waiting extended periods of time when endpoint isn't responding to diagnostic requests.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
    - [ ] I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool
  • I have added an integration test or an E2E test

@fearful-symmetry fearful-symmetry added the Team:Elastic-Agent Label for the Agent team label Nov 20, 2023
@fearful-symmetry fearful-symmetry self-assigned this Nov 20, 2023
@fearful-symmetry fearful-symmetry requested a review from a team as a code owner November 20, 2023 23:07
@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

Copy link
Contributor

mergify bot commented Nov 20, 2023

This pull request does not have a backport label. Could you fix it @fearful-symmetry? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@pierrehilbert
Copy link
Contributor

Test failing because of #3793

Copy link
Member

@pchila pchila left a comment

Choose a reason for hiding this comment

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

A couple of minor suggestions, code is ok otherwise.

Maybe it would be worth adding a test (doesn't have to be a full-fledged integration test, probably a simple combination of runtime manager and fake input would work) so that we can put a hard limit on how long it takes for diagnostics in case we have a component that does not support the diagnostics action ?

pkg/component/runtime/manager.go Show resolved Hide resolved
@fearful-symmetry
Copy link
Contributor Author

@pchila yah, was kinda on the fence about tests, since I wasn't sure how effective it would be to test essentially a single if statement, unless you're thinking of something else.

@pchila
Copy link
Member

pchila commented Nov 22, 2023

Well I was thinking of adding a test here to detect if code changes (not just in this PR) make elastic-agent diagnostics wait a lot longer in case some component does not implement the diagnostic action.
The idea is to reproduce the bug/usecase conditions and test for expected behavior, not the code that patches the bug.
We don't have guidelines about test coverage beyond a simple measurement so it's up to you to choose if you want to add a test here

Copy link

@fearful-symmetry fearful-symmetry merged commit 3afa073 into elastic:main Nov 27, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants