Skip to content

Add var.storage_account_container in Azure Logs #3877

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

Merged

Conversation

aspacca
Copy link
Contributor

@aspacca aspacca commented Jul 28, 2022

Enhancement

What does this PR do?

Add var.storage_account_container in Azure Logs

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.

Author's Checklist

  • [ ]

Related issues

Screenshots

The new "Storage Account Container" option is positioned in the advanced section of each data stream:

CleanShot 2022-07-29 at 13 15 55@2x

@aspacca aspacca requested a review from a team as a code owner July 28, 2022 04:10
@aspacca aspacca self-assigned this Jul 28, 2022
@elasticmachine
Copy link

elasticmachine commented Jul 28, 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-07-29T11:34:12.832+0000

  • Duration: 14 min 51 sec

Test stats 🧪

Test Results
Failed 0
Passed 105
Skipped 0
Total 105

🤖 GitHub comments

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

  • /test : Re-trigger the build.

@zmoog zmoog self-assigned this Jul 28, 2022
@zmoog zmoog added enhancement New feature or request Team:Cloud-Monitoring Label for the Cloud Monitoring team labels Jul 28, 2022
@zmoog zmoog assigned zmoog and unassigned zmoog Jul 28, 2022
@zmoog zmoog marked this pull request as draft July 28, 2022 09:24
@zmoog
Copy link
Contributor

zmoog commented Jul 28, 2022

We need to make some changes: defining the storage_account_container at the top level in the integration settings makes the same container available to all azureeventhub inputs, but integrations must not share the same storage account container across multiple azureeventhub inputs.

Each azureeventhub input uses a storage account container to store checkpoint information about its consumer group, for example:

{
    "partitionID": "0",
    "epoch": 1,
    "owner": "b5454f29-881f-48ae-bd34-5711932f378e",
    "checkpoint": {
        "offset": "47244772552",
        "sequenceNumber": 986,
        "enqueueTime": "2022-07-20T08:17:37.523Z"
    },
    "state": "available",
    "token": "923168d8-bc7a-4e75-8f70-4afb83637c12"
}

@aspacca
Copy link
Contributor Author

aspacca commented Jul 28, 2022

but integrations must not share the same storage account container across multiple azureeventhub inputs

good catch! thanks for taking care of this

I am positioning it in the "advanced" section because it requires
a deeper knowledge to set it properly and avoid side effects.
@elasticmachine
Copy link

elasticmachine commented Jul 28, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (7/7) 💚
Files 81.25% (13/16) 👎 -15.882
Classes 81.25% (13/16) 👎 -15.882
Methods 80.451% (107/133) 👎 -8.8
Lines 81.826% (1855/2267) 👎 -8.775
Conditionals 100.0% (0/0) 💚

@zmoog
Copy link
Contributor

zmoog commented Jul 28, 2022

From the technical perspective, to keep things working as expected, we must place the storage_account_container option in the data stream scope (1).

From the UI perspective, we have the choice: can make it always visible and put it in the "standard" section (2) or put it in the "advanced" section (3) of the data stream.

Since this is a vital option and can mess things up if used by multiple integrations, I am inclined to pick the "advanced" option.

I'd love to hear what you think.

cc @ravikesarwani @kaiyan-sheng

CleanShot 2022-07-28 at 16 53 09@2x

The description is also a critical aspect of this change. I started with this description (this is a draft, do not consider it final):

This is the container in the storage account where the integration stores the checkpoint data for the consumer group. The integration generates a default container name if not specified. Each integration MUST have a dedicated container; do not reuse the same container for multiple integrations.

The goal is to convey the following information:

  • what this is
  • what value should you expect
  • it is crucial not to reuse the same value in multiple integrations

@kaiyan-sheng
Copy link
Contributor

kaiyan-sheng commented Jul 28, 2022

@zmoog I also vote for putting it under the advanced options. But instead of having it all the way to the bottom of the list, I would put it before tags and processors since tags and processors are common across all data streams basically.

btw this can be considered separately. We can also add the Optional tag for each config parameter so that users know which ones are required and which ones are not. For example
Screen Shot 2022-07-28 at 9 55 47 AM

Edit: I see some fields are already specified as optional for azure. I guess we are missing the Optional tag here then?

@ravikesarwani
Copy link

Yes, let's put it in the Advanced option please since this will be used only in some use cases.

@zmoog
Copy link
Contributor

zmoog commented Jul 29, 2022

Sounds good; let's go for the "advanced" option. I expect this option to be used in rare cases, but it needs to be there for a few.

Positioning it before the tags looks like a better alternative; thanks @kaiyan-sheng! I will double-check the use of the Optional tag here.

I also moved other data stream options to match the order they appear
on the settings page.
@zmoog zmoog marked this pull request as ready for review July 29, 2022 12:43
@zmoog
Copy link
Contributor

zmoog commented Jul 29, 2022

Updated the PR description with the a screenshot of the up-to-date version of integration settings.

Copy link
Contributor

@tommyers-elastic tommyers-elastic left a comment

Choose a reason for hiding this comment

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

nice work - the description of the new field is super clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Team:Cloud-Monitoring Label for the Cloud Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants