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/sqlserver] Add SQL Server Metrics Receiver #9252

Merged
merged 1 commit into from
Apr 15, 2022

Conversation

StefanKurek
Copy link
Contributor

Description:
Add SQL Server Metrics receiver using windows performance counter as a base.

Link to tracking Issue:
#8398

Testing:
Added config testing, factory tests, and scraper unit tests to validate new code.

An integration test has been added (and was tested locally) to verify the receiver can collect metrics, but I am expecting it to not work in this pipeline off the bat as I don't think SQL Server is installed on the Windows machines provided by GitHub Actions. And I'm not sure if we want to change the behavior of the pipeline to install SQL Server every time.

Documentation:
Added doc explaining use of receiver.

@StefanKurek StefanKurek requested review from a team and Aneurysm9 April 13, 2022 14:32
@djaglowski
Copy link
Member

Some formatting checks are failing. These make targets should help debug locally.

@StefanKurek StefanKurek force-pushed the sqlserver_receiver branch 5 times, most recently from 891c367 to 79c7082 Compare April 15, 2022 01:37
@StefanKurek StefanKurek requested a review from djaglowski April 15, 2022 12:48
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.

Looks quite good, just a few minor things to tidy up.

Noting that Installing SQL Server adds 6 minutes to the build-and-test-windows workflow. I think this is a reasonable cost for the benefit of being able to run real tests for this receiver. It doesn't currently extend the total CI runtime at this point either, because parallel workflows such as build-and-test are running multiple times longer. If this workflow becomes the critical path for CI, it can be split apart such that tests requiring installation steps run on a parallel job.

.github/workflows/scripts/sqlserver_install.ps1 Outdated Show resolved Hide resolved
receiver/sqlserverreceiver/Makefile Outdated Show resolved Hide resolved
receiver/sqlserverreceiver/config_test.go Outdated Show resolved Hide resolved
receiver/sqlserverreceiver/config_test.go Show resolved Hide resolved
receiver/sqlserverreceiver/factory_others.go Outdated Show resolved Hide resolved
receiver/sqlserverreceiver/factory_others_test.go Outdated Show resolved Hide resolved
receiver/sqlserverreceiver/scraper.go Outdated Show resolved Hide resolved
receiver/sqlserverreceiver/scraper.go Outdated Show resolved Hide resolved
This makes use of the  windowsperfcounter watchers, using them to collect SQL Server metrics.
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 @StefanKurek!

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.

2 participants