Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

add the ability to configure app-insights export #102

Merged
merged 11 commits into from
Oct 7, 2020
Merged

add the ability to configure app-insights export #102

merged 11 commits into from
Oct 7, 2020

Conversation

bmc-msft
Copy link
Contributor

@bmc-msft bmc-msft commented Oct 5, 2020

Summary of the Pull Request

Adds the ability to enable automatic log export from the instance specific appinsights

PR Checklist

  • Applies to work item: Enable auto-export from appinsights #44
  • CLA signed. If not, go over here and sign the CLI.
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Info on Pull Request

Adds the --export_appinsights to the deployment script to setup app insight exports to the app-insights container on the func storage account. Note, it will automatically replace any existing app-insights exports to the same container prior to creating the export config.

Validation Steps Performed

Deploy with --export_appinsights, run for a while. Note new files are created in app-insights container on func storage account.

@bmc-msft bmc-msft linked an issue Oct 5, 2020 that may be closed by this pull request
@bmc-msft bmc-msft marked this pull request as ready for review October 5, 2020 22:05
if container_name not in [x["name"] for x in client.list_containers()]:
client.create_container(container_name)

expiry = datetime.utcnow() + timedelta(days=2 * 365)
Copy link
Member

Choose a reason for hiding this comment

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

Can we accomplish this with something other than an ad hoc (thus irrevocable) SAS URL?

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively: use a much shorter-lived URL, and have a mechanism for refreshing within an instance (though that feels a bit roundabout).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API unfortunately requires SAS urls.

Doing it through the instance requires granting more privileges to the service.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. Given the restrictions on perms, and the fact that none of our code touches it (it is only given directly to an Azure API, here), this seems okay to me. Can we just document our usage assumptions? In particular, note that this SAS URL must not "leave this script", except to the extent that it implicitly does in its use in the export request.

@bmc-msft bmc-msft merged commit 16331fc into microsoft:main Oct 7, 2020
@bmc-msft bmc-msft deleted the export-app-insights branch October 7, 2020 14:32
@ghost ghost locked as resolved and limited conversation to collaborators Apr 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable auto-export from appinsights
3 participants