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 APM trace configuration to V2AgentManager #40030

Merged
merged 6 commits into from
Jun 28, 2024

Conversation

blakerouse
Copy link
Contributor

@blakerouse blakerouse commented Jun 26, 2024

Proposed commit message

Added a APM to reload.Registry.

Adds the APM to the reload.Registry to allow a reloadable configurator to be set and automatically have Reload called when an APM trace configuration is sent over the Elastic Agent V2 control protocol.

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.next.asciidoc or CHANGELOG-developer.next.asciidoc. (no changelog as currently not user facing)

Disruptive User Impact

None.

Related issues

@blakerouse blakerouse added backport-skip Skip notification from the automated backport with mergify Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team labels Jun 26, 2024
@blakerouse blakerouse self-assigned this Jun 26, 2024
@blakerouse blakerouse requested a review from a team as a code owner June 26, 2024 15:01
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jun 26, 2024
@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jun 26, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@@ -865,6 +873,65 @@ func (cm *BeatV2Manager) reloadInputs(inputUnits []*agentUnit) error {
return nil
}

// reloadAPM reload APM tracing, it returns a bool and an error.
// The bool, if set, indicates that the output reload requires an restart,
// in that case the error is always `nil`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment seems out of date? reloadAPM doesn't return anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy-and-paste error, will fix.

@@ -669,6 +673,10 @@ func (cm *BeatV2Manager) reload(units map[unitKey]*agentUnit) {
cm.logger.Errorw("setting output state", "error", err)
}

// reload APM tracing configuration
// all error handling is handled inside of reloadAPM
Copy link
Contributor

Choose a reason for hiding this comment

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

Both reloadInputs and reloadOutput return an error, why does reloadAPM not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because in no case, do we want APM trace configuration to cause the beat to fail. I don't think we want that, so I handle all of that logic internally inside of reloadAPM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the docstring on reloadAPM to provide this reasoning.

@blakerouse blakerouse merged commit 0bb4e10 into elastic:main Jun 28, 2024
121 checks passed
cm.logger.Debug("Reloading APM tracing")
err = apm.Reload(reloadConfig)
if err != nil {
cm.logger.Debugf("Error reloading APM tracing: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: shouldn't this be logged as an error, similar to line 918?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send APMConfig to registered apm reloaders when the config has changed
5 participants