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.

Sign up for GitHub

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

Jump to bottom

Don't run any agent service when running otel subcommand #5748

Merged
merged 6 commits into from
Oct 15, 2024
Merged

Don't run any agent service when running otel subcommand #5748

merged 6 commits into from
Oct 15, 2024

Conversation

blakerouse
Copy link
Contributor

@blakerouse blakerouse commented Oct 9, 2024 •
edited
Loading

What does this PR do?

Previously the control protocol was started when the subcommand otel was ran. This should not be done as the subcommand otel should operate it in a pure OTel mode.

This is a cleanup that comes out of working on Hybrid mode.

Why is it important?

Cleanup to make the code cleaner and less messy as the control protocol should not really be running when running the otel subcommand.

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

Disruptive User Impact

Can no longer call elastic-agent status when a elastic-agent otel is running.

How to test this PR locally

Run elastic-agent otel, then try to run elastic-agent status and see that it doesn't connect.

@blakerouse blakerouse added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Oct 9, 2024
@blakerouse blakerouse self-assigned this Oct 9, 2024
@blakerouse blakerouse requested a review from a team as a code owner October 9, 2024 17:02
@elasticmachine
Copy link
Contributor

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 9, 2024

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

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 9, 2024

backport-v8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 9, 2024
@elastic-sonarqube Elastic SonarQube
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube

swiatekm
swiatekm approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@swiatekm swiatekm 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 it'd be good for @andrzej-stencel to have a look at this as well, imo.

@blakerouse blakerouse mentioned this pull request Oct 11, 2024
Merged
6 tasks
@ycombinator ycombinator mentioned this pull request Oct 15, 2024
Closed
cmacknz
cmacknz reviewed Oct 15, 2024
View reviewed changes
testing/integration/otel_test.go
@@ -463,22 +426,6 @@ func TestOtelAPMIngestion(t *testing.T) {
)
require.NoError(t, err, "APM not initialized")

// wait for otel collector to start
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth replacing this with a check of the upstream health endpoint?

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 don't think its worth it at the moment. The work I am doing in #5767 will expose status information over the elastic-agent status command again.

cmacknz
cmacknz approved these changes Oct 15, 2024
View reviewed changes
Copy link
Member

@cmacknz cmacknz left a comment

Choose a reason for hiding this comment

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

I like this as a simplification now that the direction we are going with elastic-agent is clearer than it was when the otel command was first implemented.

Once #5767 is implemented we can probably remove the elastic-agent otel command entirely.

@blakerouse
Copy link
Contributor Author

I like this as a simplification now that the direction we are going with elastic-agent is clearer than it was when the otel command was first implemented.

Once #5767 is implemented we can probably remove the elastic-agent otel command entirely.

I think it depends on if we want a true 100% otel implementation or something that has some of the Elastic Agent code running. I think the longer term goal is to make the run become otel sub-command, but at the moment we are not that far along.

@ycombinator
Copy link
Contributor

Force merging to get around the failed SonarQube check.

@ycombinator ycombinator merged commit 3f1e4da into elastic:main Oct 15, 2024
13 of 14 checks passed
mergify bot pushed a commit that referenced this pull request Oct 15, 2024
@blakerouse @mergify
Don't run any agent service when running otel subcommand (#5748)
0fb18f8 
* Remove need to run agent while running otel subcommand.

* Cleanup.

* Fix tests.

* Fix tests.

* Few more cleanups.

* Mage check.

(cherry picked from commit 3f1e4da)
@mergify mergify bot mentioned this pull request Oct 15, 2024
Merged
4 tasks
blakerouse added a commit that referenced this pull request Oct 15, 2024
@mergify @blakerouse
Don't run any agent service when running otel subcommand (#5748) (#…
05e0f08 
…5797)

* Remove need to run agent while running otel subcommand.

* Cleanup.

* Fix tests.

* Fix tests.

* Few more cleanups.

* Mage check.

(cherry picked from commit 3f1e4da)

Co-authored-by: Blake Rouse <blake.rouse@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swiatekm swiatekm swiatekm approved these changes

@cmacknz cmacknz cmacknz approved these changes

@michalpristas michalpristas Awaiting requested review from michalpristas

@andrzej-stencel andrzej-stencel Awaiting requested review from andrzej-stencel

Assignees

@blakerouse blakerouse

Labels
backport-8.x Automated backport to the 8.x branch with mergify skip-changelog Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@blakerouse @elasticmachine @ycombinator @cmacknz @swiatekm