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

comp/trace: wrap pkg/trace/agent as a component #19466

Merged
merged 57 commits into from Oct 10, 2023

Conversation

darccio
Copy link
Contributor

@darccio darccio commented Sep 14, 2023

What does this PR do?

Introduce a wrapper for pkg/trace/agent as a component. Important things to know:

  • Almost all original agent's code is still in pkg/trace/agent. Some bits have been moved mainly to comp/trace/agent/agent.go, to encapsulate them as part of the agent's lifecycle.
  • The one big exception is cmd/trace-agent/subcommands/run/run.go. Now it lives inside the component, as it contains logic to start several non-components that the agent requires. They are potential next candidates to become components.

For future reference, the gist about creating components (at least wrappers like this one) is:

  • You need to think of components like a restaurant, it has providers that user required by multiple parts of our component (internal code or other components) and it has three phases: setup, start & stop.
  • Setup is fulfilled using a provider, a function encapsulated in a fx.Module and annotated with fx.Provide, and the values/components provided by other providers. The function must return a value matching the exported Component interface in your package. It can be an empty interface.
  • Components can require other provided components and supplied values. Defined in the dependencies struct, that must embed fx.In type,the provider function must receive this struct as argument.
  • Start is done through an unexported method start registered as a lifecycle hook. Beware of the provided context, as it has a timeout. Don't use it for any long-living code, and also you must not block inside this block.
  • Stop is done in the same fashion as start, but the method is called stop. The same conditions apply regarding the provided context.
  • If your component must handle signals to interrupt the component, use the injected fx.Shutdowner when catching a signal that means you need to stop.

Additional Notes

  • It was not possible to test the command line parsing as now we use fxutil.Run instead of fxutil.OneShot.
  • Make sure you have pyinvoke installed. You need to run inv lint-components --fix before pushing, so you can avoid a failing CI.

Possible Drawbacks / Trade-offs

  • Introducing possible bugs due to unexpected changes in the processing of the configuration or the altered execution order of some parts (like CPU & memory profiling), or not using the provided context when running as Windows service.

Describe how to test/QA your changes

  • Run it in testbed environment.
  • Dogfood it in local.

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Use the major_change label if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.
  • A release note has been added or the changelog/no-changelog label has been applied.
  • Changed code has automated tests for its functionality.
  • Adequate QA/testing plan information is provided if the qa/skip-qa label is not applied.
  • At least one team/.. label has been applied, indicating the team(s) that should QA this change.
  • If applicable, docs team has been notified or an issue has been opened on the documentation repo.
  • If applicable, the need-change/operator and need-change/helm labels have been applied.
  • If applicable, the k8s/<min-version> label, indicating the lowest Kubernetes version compatible with this feature.
  • If applicable, the config template has been updated.

@bits-bot
Copy link
Collaborator

bits-bot commented Sep 14, 2023

CLA assistant check
All committers have signed the CLA.

comp/trace/agent/agent.go Outdated Show resolved Hide resolved
comp/trace/config/config_mock.go Show resolved Hide resolved
cmd/trace-agent/subcommands/run/command.go Outdated Show resolved Hide resolved
comp/trace/agent/agent.go Outdated Show resolved Hide resolved
comp/trace/agent/component.go Outdated Show resolved Hide resolved
comp/trace/bundle_mock.go Show resolved Hide resolved
comp/trace/bundle_test.go Outdated Show resolved Hide resolved
@darccio darccio marked this pull request as ready for review September 19, 2023 15:41
@darccio darccio requested review from a team as code owners September 19, 2023 15:41
@darccio
Copy link
Contributor Author

darccio commented Sep 19, 2023

Ready for review, but probably tests will fail. I'll fix them tomorrow.

Copy link
Contributor

@katiehockman katiehockman left a comment

Choose a reason for hiding this comment

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

Overall, looks good. Just a few minor comments.

Can you add more details to the PR description, in addition to filling out the sections in the existing template? I feel that using the components framework is pretty nuanced, and it would be very helpful to have more details around what decision decisions were made here, what shared components the trace-agent is now using, which it isn't, future work planned, etc.

comp/trace/agent/agent.go Outdated Show resolved Hide resolved
comp/trace/agent/agent.go Show resolved Hide resolved
comp/trace/agent/agent.go Outdated Show resolved Hide resolved
comp/trace/agent/agent.go Outdated Show resolved Hide resolved
comp/trace/agent/agent.go Outdated Show resolved Hide resolved
comp/trace/agent/agent.go Outdated Show resolved Hide resolved
@darccio darccio added the team/agent-apm trace-agent label Sep 20, 2023
@darccio darccio force-pushed the dario.castane/AIT-8301/tracer-agent-component branch from cfbe760 to fb8cb37 Compare October 4, 2023 15:08
@darccio darccio force-pushed the dario.castane/AIT-8301/tracer-agent-component branch from 5f72f26 to 479c4b9 Compare October 5, 2023 11:15
@darccio darccio force-pushed the dario.castane/AIT-8301/tracer-agent-component branch from 479c4b9 to ad2a185 Compare October 5, 2023 15:07
Copy link
Contributor

@ogaca-dd ogaca-dd left a comment

Choose a reason for hiding this comment

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

LGTM but I think not returning a nil component is important.

cmd/trace-agent/subcommands/run/command.go Outdated Show resolved Hide resolved
comp/trace/bundle_test.go Outdated Show resolved Hide resolved
cmd/trace-agent/subcommands/run/command.go Show resolved Hide resolved
comp/trace/agent/agent.go Outdated Show resolved Hide resolved
comp/trace/agent/component.go Outdated Show resolved Hide resolved
comp/trace/agent/component.go Outdated Show resolved Hide resolved
comp/trace/agent/agent.go Outdated Show resolved Hide resolved
comp/trace/agent/agent.go Outdated Show resolved Hide resolved
Copy link
Contributor

@chouquette chouquette left a comment

Choose a reason for hiding this comment

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

LGTM for agent-platform

@darccio darccio added the qa/skip-qa [deprecated] [DEPRECATED] Please use qa/done or qa/no-code-change to skip creating a QA card label Oct 10, 2023
@darccio darccio merged commit 779514d into main Oct 10, 2023
132 checks passed
@darccio darccio deleted the dario.castane/AIT-8301/tracer-agent-component branch October 10, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog component framework qa/skip-qa [deprecated] [DEPRECATED] Please use qa/done or qa/no-code-change to skip creating a QA card team/agent-apm trace-agent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants