Implement Data Collection Rule for Container Insights #623

zioproto · 2024-12-19T17:25:09Z

Describe your changes

Implement Data Collection Rule for Container Insights. This completes the work started in PR #446

Issue number

#607

Checklist before requesting a review

The pr title can be used to describe what this pr did in CHANGELOG.md file
I have executed pre-commit on my machine
I have passed pr-check on my machine

zioproto · 2024-12-19T17:30:00Z

@lonegunmanb this PR was implemented looking at the documentation example available here:

https://github.com/microsoft/Docker-Provider/tree/ci_prod/scripts/onboarding/aks/onboarding-msi-terraform-syslog

Cc: @ganga1980 and @wanlonghenry in case they have review comments. Thanks

lonegunmanb

@zioproto We've got the following error:

TestExamplesForV4/application_gateway_ingress_v4 2024-12-20T16:01:45Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: exit status 1; ╷
│ Error: Attempt to get attribute from null value
│ 
│   on ../../v4/log_analytics.tf line 86, in locals:
│   86:   dcr_location = coalesce(local.log_analytics_workspace.location, try(data.azurerm_log_analytics_workspace.main[0].location, null))
│     ├────────────────
│     │ local.log_analytics_workspace is null
│ 
│ This value is null, so it does not have any attributes.
╵}
    apply.go:34: 
        	Error Trace:	/src/test/vendor/github.com/gruntwork-io/terratest/modules/terraform/apply.go:34
        	            				/src/test/vendor/github.com/Azure/terraform-module-test-helper/e2etest.go:107
        	            				/src/test/vendor/github.com/Azure/terraform-module-test-helper/e2etest.go:87
        	            				/src/test/vendor/github.com/Azure/terraform-module-test-helper/e2etest.go:55
        	            				/src/test/e2e/terraform_aks_test.go:211
        	Error:      	Received unexpected error:
        	            	FatalError{Underlying: error while running command: exit status 1; ╷
        	            	│ Error: Attempt to get attribute from null value
        	            	│ 
        	            	│   on ../../v4/log_analytics.tf line 86, in locals:
        	            	│   86:   dcr_location = coalesce(local.log_analytics_workspace.location, try(data.azurerm_log_analytics_workspace.main[0].location, null))
        	            	│     ├────────────────
        	            	│     │ local.log_analytics_workspace is null
        	            	│ 
        	            	│ This value is null, so it does not have any attributes.
        	            	╵}
        	Test:       	TestExamplesForV4/application_gateway_ingress_v4

Need further investigation.

lonegunmanb · 2024-12-25T09:04:09Z

log_analytics.tf

 }
+
+locals {
+  dcr_location = coalesce(local.log_analytics_workspace.location, try(data.azurerm_log_analytics_workspace.main[0].location, null))


Could we try coalesce(try(local.log_analytics_workspace.location, null), try(data.azurerm_log_analytics_workspace.main[0].location, null)) here?

zioproto · 2025-01-09T08:47:37Z

@lonegunmanb I managed to find the correct expression, the E2E tests are now passing. Please have a look. Thanks

lonegunmanb

Thanks @zioproto , some comments

lonegunmanb · 2025-03-03T09:38:35Z

log_analytics.tf

+  data_sources {
+    syslog {
+      streams        = ["Microsoft-Syslog"]
+      facility_names = var.syslog_facilities


Could we rename this new variable to monitor_data_collection_rule_data_sources_syslog_facilities?

lonegunmanb · 2025-03-03T09:38:48Z

log_analytics.tf

+    syslog {
+      streams        = ["Microsoft-Syslog"]
+      facility_names = var.syslog_facilities
+      log_levels     = var.syslog_levels


Could we rename this new variable to monitor_data_collection_rule_data_sources_syslog_levels?

lonegunmanb · 2025-03-03T09:39:12Z

log_analytics.tf

+    }
+
+    extension {
+      streams        = var.streams


Could we rename this new variable to monitor_data_collection_rule_extensions_streams?

lonegunmanb · 2025-03-03T09:41:10Z

log_analytics.tf

+          "namespaces" : var.data_collection_settings.namespaces_for_data_collection,
+          "enableContainerLogV2" : var.data_collection_settings.container_log_v2_enabled,
+        }
+      })


We could use HCL syntax here:

{ dataCollectionSettings = { interval = var.data_collection_settings.data_collection_interval namespaceFilteringMode = var.data_collection_settings.namespace_filtering_mode_for_data_collection namespaces = var.data_collection_settings.namespaces_for_data_collection enableContainerLogV2 = var.data_collection_settings.container_log_v2_enabled } }

lonegunmanb · 2025-03-03T09:42:09Z

log_analytics.tf

+}
+
+resource "azurerm_monitor_data_collection_rule" "dcr" {
+  count               = (var.log_analytics_workspace_enabled && var.oms_agent_enabled) ? 1 : 0


Would count = (local.create_analytics_workspace && var.oms_agent_enabled) ? 1 : 0 be better?

lonegunmanb · 2025-03-03T09:42:50Z

log_analytics.tf

+}
+
+resource "azurerm_monitor_data_collection_rule_association" "dcra" {
+  count                   = (var.log_analytics_workspace_enabled && var.oms_agent_enabled) ? 1 : 0


Would count = (local.create_analytics_workspace && var.oms_agent_enabled) ? 1 : 0 be better?

lonegunmanb

Thanks @zioproto, LGTM!

zioproto had a problem deploying to test December 19, 2024 17:25 — with GitHub Actions Failure

zioproto requested a review from lonegunmanb December 19, 2024 17:25

zioproto force-pushed the container-insights-dcr branch from 6d4a17e to 74b27bc Compare December 19, 2024 17:27

zioproto had a problem deploying to test December 19, 2024 17:27 — with GitHub Actions Failure

zioproto force-pushed the container-insights-dcr branch from 74b27bc to 9c5f699 Compare December 20, 2024 10:12

zioproto had a problem deploying to test December 20, 2024 10:13 — with GitHub Actions Failure

zioproto force-pushed the container-insights-dcr branch from 9c5f699 to 95da31f Compare December 20, 2024 15:29

zioproto had a problem deploying to test December 20, 2024 15:29 — with GitHub Actions Failure

lonegunmanb requested changes Dec 25, 2024

View reviewed changes

zioproto force-pushed the container-insights-dcr branch from 95da31f to 6e4c7a6 Compare January 6, 2025 07:44

zioproto had a problem deploying to test January 6, 2025 07:44 — with GitHub Actions Failure

zioproto force-pushed the container-insights-dcr branch from 6e4c7a6 to d7fcda5 Compare January 6, 2025 08:22

zioproto had a problem deploying to test January 6, 2025 08:22 — with GitHub Actions Failure

zioproto force-pushed the container-insights-dcr branch from d7fcda5 to b70b241 Compare January 6, 2025 13:24

zioproto had a problem deploying to test January 6, 2025 13:24 — with GitHub Actions Failure

zioproto force-pushed the container-insights-dcr branch from b70b241 to 5af1c80 Compare January 8, 2025 11:48

zioproto had a problem deploying to test January 8, 2025 11:48 — with GitHub Actions Failure

zioproto force-pushed the container-insights-dcr branch from 5af1c80 to e40655b Compare January 8, 2025 16:56

zioproto had a problem deploying to test January 8, 2025 16:57 — with GitHub Actions Failure

zioproto force-pushed the container-insights-dcr branch from e40655b to 6b92981 Compare January 9, 2025 06:28

zioproto temporarily deployed to test January 9, 2025 06:29 — with GitHub Actions Inactive

lonegunmanb requested changes Mar 3, 2025

View reviewed changes

zioproto and others added 2 commits March 20, 2025 10:17

Implement Data Collection Rule for Container Insights

00b4145

improve naming

8c6fd61

lonegunmanb force-pushed the container-insights-dcr branch from 6b92981 to 8c6fd61 Compare March 20, 2025 02:24

lonegunmanb had a problem deploying to test March 20, 2025 02:25 — with GitHub Actions Failure

pre commit

3a94a33

lonegunmanb had a problem deploying to test March 20, 2025 02:35 — with GitHub Actions Failure

fix incorrect config

5d11d95

lonegunmanb temporarily deployed to test March 20, 2025 02:42 — with GitHub Actions Inactive

lonegunmanb approved these changes Mar 20, 2025

View reviewed changes

lonegunmanb merged commit 252c132 into main Mar 20, 2025
5 checks passed

github-project-automation bot moved this from Todo to Done in Azure Module Kanban Mar 20, 2025

lonegunmanb deleted the container-insights-dcr branch March 20, 2025 04:44

lonegunmanb mentioned this pull request Apr 15, 2025

Container insights not working when using existing Log Analytics Workspace #607

Closed

1 task

Implement Data Collection Rule for Container Insights #623

Implement Data Collection Rule for Container Insights #623

Uh oh!

Conversation

zioproto commented Dec 19, 2024

Describe your changes

Issue number

Checklist before requesting a review

Uh oh!

zioproto commented Dec 19, 2024

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

lonegunmanb Dec 25, 2024

Choose a reason for hiding this comment

Uh oh!

zioproto commented Jan 9, 2025

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

lonegunmanb Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

lonegunmanb Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

lonegunmanb Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

lonegunmanb Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

lonegunmanb Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

lonegunmanb Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants