[AZINTS-3933] Use ARG to collect existing Log Forwarders#9
Conversation
|
✅ Code Quality ✅ Code Vulnerabilities ✅ Library Vulnerabilities 🎉 All green!🛠️ No new code quality issues 🔗 Commit SHA: 9848630 | Docs | Was this helpful? Give us feedback! |
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| if subscriptions is not None: | ||
| if len(subscriptions) == 0: | ||
| return {} # searching empty set of subscriptions | ||
| subscriptions_clause = " and subscriptionId in ({})".format(", ".join(["'{}'".format(subscription_id) for subscription_id in subscriptions])) |
There was a problem hiding this comment.
3.9 doesn't have f-strings? 🤔
There was a problem hiding this comment.
It does but we're nesting and also using quotes in the string.
There was a problem hiding this comment.
You could probably make use of single and double quotes to make it work but doesn't matter
|
|
||
|
|
||
| def execute(az_cmd: AzCmd) -> str: | ||
| def execute(az_cmd: AzCmd, can_fail: bool = False) -> str: |
There was a problem hiding this comment.
Is there a specific error string we can look for relating to the extension instead of using this flag?
There was a problem hiding this comment.
I considered that but ultimately figured we're better off leaving it open ended. We don't want it to break if the error message changes, and the risk of attempting to (re?)-install the extension when we get some other error is low - it would just lead to another error being raised and logged later, most likely when we make that attempt.
agulen
left a comment
agulen
left a comment
There was a problem hiding this comment.
Looks good overall - few small comments. This will be a nice time save 👍 Signing off to unblock
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
dd-devflow
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
dd-mergequeue
Bot
deleted the
benjamin.johnsonstaub/azints-3933/arg-for-existing-lfos
branch
3 participants
Motivation
The way we were previously searching for log forwarders involved running a separate
listaz command for every subscription we wanted to check for a log forwarder. This might be ok if we were limiting ourselves to a single management group to avoid conflicts with the current version of the LFO script, but we are trying to soft enforce 1 Log Forwarder per customer moving forward. The UI is built to be compatible only with this case.Therefore, we want to know if there are any log forwarders anywhere in the tenant, which means 1 az command per subscription the user can see... In practice this was very slow, exhibiting an inexplicable bimodal distribution around 18 and 80 seconds, the latter being completely unacceptable for the UI.
By using ARG we can speed this up considerably.
Summary
Pulls the LFO control plane search logic into its own function that uses a single ARG query instead of listing
functionapps in every subscription. This optionally takes a set of subscriptions to search (which will be passed in the context of the main LFO script, for now, to maintain the status quo), or searches all subscriptions by default.This also lets the quickstart script grab the control plane metadata without waiting for monitored subscriptions, which we don't need.
ARG requires an az cli extension, so we check if that's installed before making the query, and install it if it isn't. I had to make some changes to the implementation of
executeso that we don't report an error when our "check" exits with non-zero status, an expected result. It's not the most elegant, but the alternative seemed to be duplicating quite a bit of code.Testing
I've modified some of the existing lfo unit/integration tests and these all pass. I've also built the integration quickstart artifact and ran the UI against it and verified both that it works and that performance is considerably increased, down to a couple seconds!