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

Add component-level diagnostics to API #80

Merged

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Jul 19, 2023

Part of elastic/elastic-agent#2141 and elastic/elastic-agent#2140

This PR contains a few changes:

  • Adds a level field to the ActionRequest message type that tags the action as acting on either a unit or the whole component. This contains three level types, with the first being ALL in order to make sure that older non-compatible clients will behave in the expected way. Right now, only diagnostics care about this field.
  • Alters the behavior of tryPerformDiagnostics to support the level field; diagnostics that are registered at the client-level become associated with the component level, and diagnostics registered with a unit become associated with the unit level.
  • Adds the concept of a optional diagnostic via RegisterOptionalDiagnosticHook(). These optional diagnostics are triggered by corresponding tag in the params field of the ActionRequest message.
  • Adds an exported DiagnosticParams that allows the diagnostic actions to take advantage of the params field in a standardized way.
  • Adds a CPU profile diagnostic tagged as optional.
  • Add a number of tests.

Additional things to consider:

  • I'm sure if it's worth expanding DiagnosticParams in additional ways while we're here; there's easily a few forward-looking field we could add, but right now this was strictly focused on supporting optional CPU profiling.
  • I made the ActionRequest changes with the goal of making changes both non-breaking and relatively consistent with other parts of the API, but I'm not particularly happy with stuff like If level=component, then the consumer should ignore the unit_id and unit_type fields. My other idea was to break apart the ActionRequest message into different types entirely, perhaps with a different API request for diagnostics, as opposed to the currently strategy of using a single API call for actions and diagnostics. That, however, would be a much larger change.

@fearful-symmetry fearful-symmetry added enhancement New feature or request Team:Elastic-Agent Label for the Agent team labels Jul 19, 2023
@fearful-symmetry fearful-symmetry self-assigned this Jul 19, 2023
@fearful-symmetry fearful-symmetry changed the title Add compoonent-level diagnostics to API Add component-level diagnostics to API Jul 19, 2023
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

cc @fearful-symmetry

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-07-19T16:29:05.343+0000

  • Duration: 4 min 3 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Looks good.

Clearly this is backwards compatible change with the ALL for the level, so that is great! I think the splitting of this will provide a clear benefit and will hopefully remove the duplication of some diagnostic information to ever unit when its for the entire component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants