-
Notifications
You must be signed in to change notification settings - Fork 497
[microsoft_sqlserver] Add transaction log DataStream #3395
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 transaction log DataStream #3395
Conversation
🌐 Coverage report
|
Pinging @elastic/integrations (Team:Integrations) |
packages/microsoft_sqlserver/data_stream/transaction_log/fields/fields.yml
Outdated
Show resolved
Hide resolved
packages/microsoft_sqlserver/data_stream/transaction_log/fields/fields.yml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add ecs.yml file
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
@ManojS-shetty @muthu-mps, @ruflin @jsoriano @r00tu53r, tar-getting the merge of this PR, need your approval for the same. |
The changes are done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good in general, but I am concerned about some changes in versions.
packages/microsoft_sqlserver/data_stream/transaction_log/fields/base-fields.yml
Outdated
Show resolved
Hide resolved
packages/microsoft_sqlserver/data_stream/transaction_log/fields/fields.yml
Show resolved
Hide resolved
packages/microsoft_sqlserver/data_stream/transaction_log/fields/fields.yml
Outdated
Show resolved
Hide resolved
- name: driver | ||
type: keyword | ||
description: Driver used to execute the query. | ||
- name: query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, do we need the query at all in the final document? This is an implementation detail of the integration, I think that it could be removed.
But, if we are storing metrics coming from different queries in different documents we may need some dimension to distinguish between queries. So we may need to index the query to use it as dimension, or find some other field that could cover this.
We can also wait to have some way to join events (elastic/beats#31806 (comment)), then database_id
will be probably enough as dimension.
packages/microsoft_sqlserver/data_stream/transaction_log/fields/fields.yml
Show resolved
Hide resolved
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for addressing all comments.
The main concern I would have now is about mssql.query
. It is not clear to me if we are keeping it at the end or not.
Also I think that this data stream would benefit of elastic/beats#31806, to store all the metrics in the same document. And if we end up storing the results of the queries in the same document, what would we store in mssql.query
? Would we remove it? This would be a breaking change.
The more I think about storing the query, the more I think that we shouldn't store it. And it is easier in any case to add it later if I am wrong, than to remove it if needed later.
...es/microsoft_sqlserver/data_stream/transaction_log/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
...es/microsoft_sqlserver/data_stream/transaction_log/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
...es/microsoft_sqlserver/data_stream/transaction_log/elasticsearch/ingest_pipeline/default.yml
Show resolved
Hide resolved
packages/microsoft_sqlserver/data_stream/transaction_log/sample_event.json
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please remember to add pipeline tests at some moment
…mps/integrations into mssql_transaction_log_data_stream
What does this PR do?
Collects the transaction log metrics for Microsoft Sql server.
Please refer this PR for performance datastream.
Checklist
changelog.yml
file.MSSQL server integration using generic sql, as per the design doc
How to test this PR locally
Install the MSSQL
Screenshots