Skip to content

feat(infrastructure): enable optional AML diagnostic logs#400

Open
fbeltrao wants to merge 2 commits intomicrosoft:mainfrom
fbeltrao:feature/infra-aml-diagnostics
Open

feat(infrastructure): enable optional AML diagnostic logs#400
fbeltrao wants to merge 2 commits intomicrosoft:mainfrom
fbeltrao:feature/infra-aml-diagnostics

Conversation

@fbeltrao
Copy link
Copy Markdown
Contributor

@fbeltrao fbeltrao commented Apr 7, 2026

Description

Closes #399

Enable an opt-in AML workspace diagnostic setting that forwards all AML resource logs to the platform Log Analytics workspace. Thread should_enable_aml_diagnostic_logs from the root module into module.platform, create azurerm_monitor_diagnostic_setting.ml_workspace_logs in modules/platform/azureml.tf, and document the new flag in the Terraform reference files and example tfvars.

Add Terraform tests that verify the diagnostic setting is absent by default, present when enabled, and configured with the standard name plus category_group = "allLogs".

Type of Change

  • 🐛 Bug fix (non-breaking change fixing an issue)
  • ✨ New feature (non-breaking change adding functionality)
  • 💥 Breaking change (fix or feature causing existing functionality to change)
  • 📚 Documentation update
  • 🏗️ Infrastructure change (Terraform/IaC)
  • ♻️ Refactoring (no functional changes)

Component(s) Affected

  • infrastructure/terraform/prerequisites/ - Azure subscription setup
  • infrastructure/terraform/ - Terraform infrastructure
  • infrastructure/setup/ - OSMO control plane / Helm
  • workflows/ - Training and evaluation workflows
  • training/ - Training pipelines and scripts
  • docs/ - Documentation

Testing Performed

  • Terraform plan reviewed (no unexpected changes)
  • Terraform apply tested in dev environment
  • Training scripts tested locally with Isaac Sim
  • OSMO workflow submitted successfully
  • Smoke tests passed (smoke_test_azure.py)

Documentation Impact

  • No documentation changes needed
  • Documentation updated in this PR
  • Documentation issue filed

Bug Fix Checklist

Complete this section for bug fix PRs. Skip for other contribution types.

  • Linked to issue being fixed
  • Regression test included, OR
  • Justification for no regression test:

Checklist

- add AML workspace diagnostic setting behind a default-off Terraform flag
- update Terraform tests, example tfvars, and docs references

🔒 - Generated by Copilot
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@fbeltrao fbeltrao requested a review from a team as a code owner April 7, 2026 09:09
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 50.46%. Comparing base (acfe20d) to head (43830b9).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #400   +/-   ##
=======================================
  Coverage   50.46%   50.46%           
=======================================
  Files         267      267           
  Lines       18098    18098           
  Branches     1855     1855           
=======================================
  Hits         9134     9134           
  Misses       8674     8674           
  Partials      290      290           
Flag Coverage Δ *Carryforward flag
pester 81.96% <ø> (ø)
pytest 6.89% <ø> (ø) Carriedforward from 4a8de0a
pytest-dataviewer 61.97% <ø> (ø)
vitest 50.72% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@katriendg katriendg left a comment

Choose a reason for hiding this comment

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

Thank you @fbeltrao this is looking great. And thanks for the updates in docs already as well.
Left one minor comment, not a blocker, see if you feel it's something you want to add.

log_analytics_workspace_id = azurerm_log_analytics_workspace.main.id

enabled_log {
category_group = "allLogs"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No metric {} block: the azurerm_monitor_diagnostic_setting omits a metric {} block. Current provider version handles this silently, but adding metric { category = "AllMetrics" enabled = false } makes the intent explicit and avoids future deprecation warnings.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in 43830b9

- add explicit disabled diagnostic metric block for AML workspace settings
- align with PR review feedback while preserving passing terraform tests

�� - Generated by Copilot
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

feat(infrastructure): enable optional AML workspace diagnostic logs

3 participants