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

[microsoft_sqlserver] Add performance DataStream #3391

Merged
merged 73 commits into from
Jun 14, 2022

Conversation

ManojS-shetty
Copy link
Contributor

@ManojS-shetty ManojS-shetty commented May 19, 2022

What does this PR do?

This PR contributes the performance data stream for Microsoft sqlserver which fetches information from what’s commonly known as Performance Counters in MSSQL. Please refer for transaction_log data stream PR here transaction_log

MSSQL server integration developed using generic sql, as per the design doc

  • Added a data stream (performance) under microsoft_sqlserver integration package
  • Added data collection logic for the data streams.
  • Added Fields metadata in the appropriate yaml files.
  • Added dashboards and visualizations.
  • Added System Testing.
  • No Documentation added.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

How to test this PR locally

  • Install the MSSQL
  • Go to Integration UI in Kibana
  • Add the Microsoft SQL Server Integration
  • Enroll the new elastic-agent
  • In the asset -> Performance dashboard you should be able to see the default visualization.

Screenshots

sqlserver-perf-dashboard

@ManojS-shetty ManojS-shetty added enhancement New feature or request Team:Integrations Label for the Integrations team labels May 19, 2022
@ManojS-shetty ManojS-shetty requested a review from a team May 19, 2022 16:07
@ManojS-shetty ManojS-shetty self-assigned this May 19, 2022
@elasticmachine
Copy link

elasticmachine commented May 19, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-06-14T05:22:46.315+0000

  • Duration: 14 min 24 sec

Test stats 🧪

Test Results
Failed 0
Passed 8
Skipped 0
Total 8

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@ManojS-shetty
Copy link
Contributor Author

ManojS-shetty commented Jun 3, 2022

Lets make sure we always use vis by value: #3448

Done . Visualizations are now using by value.

@andrewkroh andrewkroh added the Integration:microsoft_sqlserver Microsoft SQL Server label Jun 3, 2022
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Added some comments about fields definitions.

@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@lalit-satapathy
Copy link
Collaborator

@ManojS-shetty @muthu-mps,
Do we have any changes pending or all the feedbacks are resolved now?

@ruflin @jsoriano @r00tu53r, tar-getting the merge of this PR, need your approval for the same.

@ManojS-shetty
Copy link
Contributor Author

@ManojS-shetty @muthu-mps, Do we have any changes pending or all the feedbacks are resolved now?

@ruflin @jsoriano @r00tu53r, tar-getting the merge of this PR, need your approval for the same.

The changes are Done.

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

There are a couple of unaddressed comments.

@jsoriano jsoriano self-requested a review June 8, 2022 15:55
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Same comments as in #3395 (review)

It'd be nice to have some pipeline tests, but I am ok with adding them later.

And I would consider removing the query by now, taking into account that it will be easier to add it later, than to remove it.

@ManojS-shetty
Copy link
Contributor Author

Same comments as in #3395 (review)

It'd be nice to have some pipeline tests, but I am ok with adding them later.

And I would consider removing the query by now, taking into account that it will be easier to add it later, than to remove it.

@jsoriano Removed Query field for now.

@ManojS-shetty
Copy link
Contributor Author

Pinging @elastic/security-external-integrations for approval.

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

I'd like to see a pipeline test here.

@ManojS-shetty
Copy link
Contributor Author

I'd like to see a pipeline test here.

Thank you for that suggestion , Yes we will be keeping the pipeline test in enhancement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:microsoft_sqlserver Microsoft SQL Server Team:Integrations Label for the Integrations team Team:Service-Integrations Label for the Service Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.