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

re-enable otel subcommand on Windows #6068

Merged
merged 7 commits into from
Nov 27, 2024
Merged

re-enable otel subcommand on Windows #6068

merged 7 commits into from
Nov 27, 2024

Conversation

leehinman
Copy link
Contributor

@leehinman leehinman commented Nov 18, 2024 •
edited
Loading

What does this PR do?

  1. Re-enables otel subcommand on Windows
  2. Moves launching go routine that processes Windows Service events earlier in boot process
  3. Adds Windows machines to otel integration tests

Why is it important?

Previously when Windows was added, elastic-agent did not respond to the Windows Service manager quickly enough that it was starting and was deemed "unresponsive". Moving the go routine that responds to the Windows Service Manager earlier in the boot process should make this less likely. Given go's design of DLL loading and init code we can't eliminate this completely.

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

None.

How to test this PR locally

mage integration:auth
mage integration:test

Related issues

Questions to ask yourself

  • How are we going to support this in production?
  • How are we going to measure its adoption?
  • How are we going to debug this?
  • What are the metrics I should take care of?
  • ...

@leehinman leehinman requested a review from a team as a code owner November 18, 2024 20:36
@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 18, 2024

This pull request does not have a backport label. Could you fix it @leehinman? 🙏
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 Nov 18, 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 Nov 18, 2024
@leehinman leehinman added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Nov 18, 2024
@elasticmachine
Copy link
Contributor

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

@leehinman leehinman force-pushed the 4976_windows_startup branch from 5b13070 to e6091b9 Compare November 19, 2024 14:42
@leehinman leehinman requested a review from a team as a code owner November 19, 2024 14:42
@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 20, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b 4976_windows_startup upstream/4976_windows_startup
git merge upstream/main
git push upstream 4976_windows_startup

leehinman
leehinman commented Nov 21, 2024
View reviewed changes
internal/pkg/otel/run.go Outdated Show resolved Hide resolved
@leehinman leehinman force-pushed the 4976_windows_startup branch 2 times, most recently from a922b9f to 42ed9c8 Compare November 22, 2024 14:54
@leehinman leehinman force-pushed the 4976_windows_startup branch from 42ed9c8 to d006214 Compare November 25, 2024 17:44
@elastic-sonarqube Elastic SonarQube
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

mauri870
mauri870 approved these changes Nov 26, 2024
View reviewed changes
Copy link
Member

@mauri870 mauri870 left a comment

Choose a reason for hiding this comment

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

Codewise looks good, tests are passing so I assume the changes did fix the issue. I guess worst case scenario is that this starts flakying on CI.

michalpristas
michalpristas approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@michalpristas michalpristas left a comment

Choose a reason for hiding this comment

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

nice optimisation, thanks for that @leehinman

internal/pkg/agent/cmd/run.go
@@ -154,6 +142,18 @@ func run(override cfgOverrider, testingMode bool, fleetInitTimeout time.Duration
defer cancel()
go service.ProcessWindowsControlEvents(stopBeat)

if err := handleUpgrade(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@leehinman leehinman merged commit 8e83ce0 into elastic:main Nov 27, 2024
14 checks passed
@leehinman leehinman deleted the 4976_windows_startup branch November 27, 2024 15:27
mergify bot pushed a commit that referenced this pull request Nov 27, 2024
@leehinman @mergify
re-enable otel subcommand on Windows (#6068)
71a886f 
* move processing windows events earlier in the boot process

* add Windows to otel integration tests

(cherry picked from commit 8e83ce0)
@mergify mergify bot mentioned this pull request Nov 27, 2024
Merged
7 tasks
pierrehilbert added a commit that referenced this pull request Dec 2, 2024
@mergify @leehinman @pierrehilbert
re-enable otel subcommand on Windows (#6068) (#6156)
48e7e3a 
* move processing windows events earlier in the boot process

* add Windows to otel integration tests

(cherry picked from commit 8e83ce0)

Co-authored-by: Lee E Hinman <[email protected]>
Co-authored-by: Pierre HILBERT <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mauri870 mauri870 mauri870 approved these changes

@cmacknz cmacknz cmacknz approved these changes

@michalpristas michalpristas michalpristas approved these changes

@michel-laterman michel-laterman Awaiting requested review from michel-laterman michel-laterman is a code owner automatically assigned from elastic/elastic-agent-control-plane

@pchila pchila Awaiting requested review from pchila pchila is a code owner automatically assigned from elastic/elastic-agent-control-plane

Assignees

@leehinman leehinman

Labels
backport-8.x Automated backport to the 8.x branch with mergify Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Milestone
No milestone
5 participants
@leehinman @elasticmachine @michalpristas @cmacknz @mauri870