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

Add Log Analytics with Solutions and Diagnostics Settings Sample #607

Closed
wants to merge 4 commits into from
Closed

Add Log Analytics with Solutions and Diagnostics Settings Sample #607

wants to merge 4 commits into from

Conversation

joshuawaddell
Copy link
Contributor

Created a sample BICEP file that deploys the following:

  • Log Analytics Workspace
  • Diagnostic Settings for the Log Analytics Workspace that highlights a child resource
  • Log Analytics Solutions for VM Insights and Container Insights

Naming convention for the resources based on ${uniqueString(resourceGroup().id)}

Copy link
Collaborator

@alex-frankel alex-frankel 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 the contribution! Can you take a look at the comments and let us know if you have questions about the changes we are requesting?

}

resource solutionsVMInsights 'Microsoft.OperationsManagement/solutions@2015-11-01-preview' = {
name: '${vmInsights.name}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't need the string interpolation here. You can simply do:

name: vmInsights.name

Copy link
Contributor Author

@joshuawaddell joshuawaddell Oct 7, 2020

Choose a reason for hiding this comment

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

updated to name: vmInsights.name

param logAnalyticsWorkspaceName string = 'la-${uniqueString(resourceGroup().id)}'

var vmInsights = {
name: 'VMInsights(${logAnalyticsWorkspaceName})'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you intend to have the parens () as part of the string here? In other words, the parens are going to be in the final calculated name and will show up in the resource names

Copy link
Contributor Author

@joshuawaddell joshuawaddell Oct 7, 2020

Choose a reason for hiding this comment

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

I did intend to have the parens () as part of the string. In referencing the documentation on enabling Azure Monitor for VMs and Azure Monitor for Containers, that syntax is used. You can view the sample ARM Template at this link: https://docs.microsoft.com/en-us/azure/azure-monitor/insights/vminsights-enable-resource-manager

"apiVersion": "2015-11-01-preview", "type": "Microsoft.OperationsManagement/solutions", "location": "[parameters('WorkspaceLocation')]", "name": "[concat('VMInsights', '(', split(parameters('WorkspaceResourceId'),'/')[8], ')')]",

I tried using name: 'VMInsights-${logAnalyticsWorkspaceName}' as well as some other options, and the deployment errored out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, ok. Looks good to me then.

@alex-frankel
Copy link
Collaborator

Also, I think tests are failing for the same reason as as #574. We may also need to hold this PR until #580 is merged

@alex-frankel
Copy link
Collaborator

Thanks for making the updates! We will wait until #580 is merged and see if the tests start passing. You may need to regenerate the bicep files with the new bicep CLI at that time.

@joshuawaddell
Copy link
Contributor Author

Thanks for making the updates! We will wait until #580 is merged and see if the tests start passing. You may need to regenerate the bicep files with the new bicep CLI at that time.

Question: For the BICEP CLI, should that be the latest download, or will I need to compile a new version of the CLI?

@alex-frankel
Copy link
Collaborator

We aren't generating nightly builds, so you need to pull the latest from here: https://github.com/Azure/bicep/actions/runs/297578556

That being said, we are in a bit of a mixed state having just introduced type validation of resources, and we are still working out the kinks. If you are alright with it, we will edit this PR to get the tests passing then merge it in. Does that sound ok?

@alex-frankel
Copy link
Collaborator

alex-frankel commented Oct 9, 2020

Closing this since these files were also in #620, which have been merged to master

@joshuawaddell joshuawaddell deleted the jowaddel/sample-log-analytics branch October 12, 2020 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants