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

[receiver/activedirectoryds] Add Active Directory Domain Services metrics receiver #9359

Merged
merged 16 commits into from
Apr 20, 2022

Conversation

BinaryFissionGames
Copy link
Contributor

@BinaryFissionGames BinaryFissionGames commented Apr 18, 2022

Description:
Adds the active_directory_ds metrics receiver, which scrapes metrics about a running Active Directory Domain Services instance.

Link to tracking Issue: #8833

Testing:

  • unit tests
  • integration test
    • Note about this integration test: It is currently infeasible to run it in GitHub actions. In order to run the receiver, a domain controller must be installed and running on the machine, which requires a machine restart (there doesn't seem to be a way to do this in GitHub actions). Otherwise, the performance counters have no associated instances, and the receiver fails to start. I have verified manually that this test does indeed work on a Windows server 2019 instance.

Documentation:

  • added README

@BinaryFissionGames BinaryFissionGames force-pushed the activedirectorydsreceiver branch from eec0289 to a88cc11 Compare April 18, 2022 19:17
@djaglowski
Copy link
Member

If I want to make a change to this receiver, what should I do to validate that I have not broken something?

@BinaryFissionGames
Copy link
Contributor Author

@djaglowski On the system you want to test on (Windows Server 2012 R2+) -

Install go + git
Install active directory and a test forest:

Install-windowsfeature -name AD-Domain-Services -IncludeManagementTools

# Local administrator requires a password for installation, otherwise the next step will fail,
# so set the Admin password if necessary:
# net user Administrator P@ssw0rd!

# Install the forest; Note that the -SafeModeAdministratorPassword is set to P@ssw0rd! in this instance, and isn't secure
Install-ADDSForest -InstallDns -DomainName test.example.com -DomainNetBIOSName TEST -SafeModeAdministratorPassword (ConvertTo-SecureString -String "P@ssw0rd!" -AsPlainText -Force) -NoRebootOnCompletion -Force

After this step, a reboot is required. The -NoRebootOnCompletion flag in that last command can be removed for the system to automatically reboot, but either way, active directory will not start until the system is rebooted.

After reboot, the integration test can be run with go test at the root of the cloned repo:

go test -tags=integration github.com/open-telemetry/opentelemetry-collector-contrib/receiver/activedirectorydsreceiver/...

As explained above, the problem is that reboot step. I've looked into windows containers, but it has a similar issue.

The only way I can think is possibly running a VM to test, but that has a few issues:

  1. I'm not sure if it's technically feasible to run a Windows VM within Github Actions
  2. It will likely add a decent chunk of time to the running of actions.
  3. I don't know the licensing implications of running a Windows VM in CI like that.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

@BinaryFissionGames, thanks for the detailed description of how to create a test system.

It seems that a true integration test is not realistic for this receiver. However, I would like to explore the idea of creating a simple simulator, rather than relying on "manual" mocking and testing. Please see other comments.

receiver/activedirectorydsreceiver/scraper.go Outdated Show resolved Hide resolved
receiver/activedirectorydsreceiver/scraper.go Outdated Show resolved Hide resolved
.github/workflows/build-and-test-windows.yml Show resolved Hide resolved
receiver/activedirectorydsreceiver/counters.go Outdated Show resolved Hide resolved
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @BinaryFissionGames

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants