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

feat(outputs.azure_data_explorer): Added support for streaming ingestion for ADX output plugin #11874

Merged
merged 6 commits into from
Oct 3, 2022

Conversation

asaharn
Copy link
Contributor

@asaharn asaharn commented Sep 23, 2022

Required for all PRs

resolves #

Added support for "managed" steaming ingestion type for Azure data explorer output plugin. The data ingestion type is now configurable. The default ingestion type will be "queued" in case no type is configured. Read more about ingestion type

Deprecations

  • localClient and localIngestor has been deprecated since the introduction of kusto.Client and ingest.Ingestor respectively in Auzre Kusto Go SDK. The change is done to maintain the consistency.
  • Because the deprications are internal, changes will not impact public usage,
  • The related deprecated code will be removed in the next update.

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Sep 23, 2022
@asaharn
Copy link
Contributor Author

asaharn commented Sep 23, 2022

Hi @srebhan,

We have created a fresh new PR that contain only the support for "managed" type ingestion for Azure ADX.

We have not changed/removed/refactored anything in the existing tests/functionality.

The changes include marking localClient and localIngestor as deprecated as they will not be used in future. These are now included as the part of Azure Kusto Go SDK. And all the related code that get deprecated will be removed in next PR.

A separate test case has been added to test the new managed and queued ingestion functionality without altering the existing testcases.

For us going forward with the functionality is on priority then just refactoring the testcases and code. Hence, this new PR

Note: This PR is in the same context with #11587 and #11697

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks for splitting the PR @asaharn! I have some small comment in the code. Please also run make fmt to keep CI happy...

I expect you to clean up the deprecated stuff in another PR once this is merged. Is this expectation correct?

plugins/outputs/azure_data_explorer/README.md Outdated Show resolved Hide resolved
plugins/outputs/azure_data_explorer/azure_data_explorer.go Outdated Show resolved Hide resolved
plugins/outputs/azure_data_explorer/azure_data_explorer.go Outdated Show resolved Hide resolved
* Address review comments
@asaharn
Copy link
Contributor Author

asaharn commented Sep 27, 2022

Hi @srebhan ,

Added the changes for the asks. Please have a look.

Additionally, running make fmt, shows no changes for ADX files (tried on Linux + Windows). It did show a change for \telegraf\plugins\inputs\processes\processes_test.go but thats out of our scope.

And yes, we will be removing the deprecated code in the next PR.

@srebhan
Copy link
Contributor

srebhan commented Sep 27, 2022

@asaharn for me CI says

[ERROR] gofmt has found errors in the following files:
plugins/outputs/azure_data_explorer/azure_data_explorer.go

Run make fmt to fix them.
make: *** [Makefile:153: fmtcheck] Error 1

are you sure you are on go 1.19?

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

@asaharn looks almost good. Thanks for your update. Please check the two remaining comments and the formatting issue.

},
}
for _, testCase := range testCases {
testCase := testCase
Copy link
Contributor

Choose a reason for hiding this comment

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

Then don't run them in parallel. :-) I don't expect a big benefit, am I mislead?

@telegraf-tiger
Copy link
Contributor

@asaharn
Copy link
Contributor Author

asaharn commented Sep 27, 2022

Hi @srebhan ,

Have added the changes and seems Ci is also happy now 😄

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for working on this @asaharn! Looking forward to your cleanup PR.

@srebhan srebhan changed the title feat: Added support for streaming ingestion for ADX output plugin feat(outputs.azure_data_explorer): Added support for streaming ingestion for ADX output plugin Sep 27, 2022
@srebhan srebhan self-assigned this Sep 27, 2022
@srebhan srebhan added area/azure Azure plugins including eventhub_consumer, azure_storage_queue, azure_monitor ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Sep 27, 2022
@asaharn
Copy link
Contributor Author

asaharn commented Oct 3, 2022

Hi @srebhan,
Can we get it merged If there is nothing pending? We will start working on cleaning up on the top of that.

@MyaLongmire
Copy link
Contributor

@asaharn sorry for the delay. prs need 2 team approvals to be merged.

@MyaLongmire MyaLongmire merged commit 9485e90 into influxdata:master Oct 3, 2022
dba-leshop pushed a commit to dba-leshop/telegraf that referenced this pull request Oct 30, 2022
@kustonaut
Copy link

Hi @srebhan @MyaLongmire is there any step missing to make this a part of next release? I noticed it could not make it through last two releases including v1.24.4

@powersj
Copy link
Contributor

powersj commented Nov 30, 2022

New features are released in new minor versions, not bug fix releases. That means it will be in v1.25.0, which should go out on or around Dec 12. For now you can use a nightly build to ensure it is working and give us early feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/azure Azure plugins including eventhub_consumer, azure_storage_queue, azure_monitor feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants