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

chore(monitoring): remove unused monitoring project #1200

Merged

Conversation

eeaton
Copy link
Collaborator

@eeaton eeaton commented Apr 25, 2024

Earlier versions of the blueprint created a monitoring project per environment, with the assumption that this could be used for creating an environment-wide metrics scoping project which would then be used by other blueprints (Enterprise Application Blueprint, MLOps blueprint). However, we haven't identified any tangible use cases for the environment wide monitoring projects, and other blueprints do not require them. The useful scope for monitoring projects will vary based on how teams monitoring related groups of workloads/applications, and requires signficicant planning and design specific to their operations, so we've decided the generic empty monitoring project does not provide significant value to a general foundation pattern.

Summary of changes:
~remove unusedmonitoring projects and other references in code
~change bootstrap steps that configure monitoring_workspace_users as a required group. Move this to optional
~remove references to the monitoring projects and related inputs/outputs in Readme text

@eeaton eeaton requested review from rjerrems, gtsorbo and a team as code owners April 25, 2024 14:38
@eeaton eeaton enabled auto-merge (squash) April 26, 2024 06:17
Copy link
Contributor

@daniel-cit daniel-cit left a comment

Choose a reason for hiding this comment

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

the terraform.example.tfvars file in 0-bootstrap needs to be updated too.
https://github.com/eeaton/terraform-example-foundation/blob/remove-env-monitoring-projects/0-bootstrap/terraform.example.tfvars#L32

Since the value of monitoring_workspace_users is not used in the code, would not be better to remove it from the optional_groups and related resources?

helpers/foundation-deployer/stages/data.go Outdated Show resolved Hide resolved
@eeaton
Copy link
Collaborator Author

eeaton commented Jun 3, 2024

Thanks for catching the instance in terraform.example.tfvars, I've updated that.

Re:

Since the value of monitoring_workspace_users is not used in the code, would not be better to remove it from the optional_groups and related resources?

Initially my thought was to leave it in optional_groups, because several of the groups in optional_groups are referenced in tests and outputs but aren't implemented in any IAM roles. This is something I plan to fix in v5, because the written guide recommends all the groups.

However, now that I think about it again, your suggestion makes sense. Even when we add more useful IAM bindings in v5, I don't expect we will have any use for the monitoring_workspace_users group so it's better to remove. I'll make an additional commit to remove it from tests and outputs and optional_groups etc.

@daniel-cit daniel-cit self-requested a review June 3, 2024 15:19
Copy link
Contributor

@daniel-cit daniel-cit left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

approved by @daniel-cit

@eeaton eeaton merged commit 7211d87 into terraform-google-modules:master Jun 5, 2024
7 checks passed
@KonStg
Copy link

KonStg commented Jun 26, 2024

There are some residual variables on

monitoring_budget_amount = optional(number, 1000)

monitoring_budget_amount
monitoring_alert_spent_percents
monitoring_alert_pubsub_topic
monitoring_budget_alert_spend_basis

@eeaton
Copy link
Collaborator Author

eeaton commented Jun 27, 2024

Thanks for the catch @KonStg . I've raised an additional PR #1281 to clean up the remaining variable definitons.

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.

4 participants