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

Addresses Issue #27 #32

Merged
merged 22 commits into from Jan 24, 2022
Merged

Addresses Issue #27 #32

merged 22 commits into from Jan 24, 2022

Conversation

awsvikram
Copy link
Contributor

No description provided.

Copy link
Member

@mhausenblas mhausenblas 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 recipe, some work required, left comments inline. Please address and re-submit.

docs/recipes/amp-alertmanager-terraform.md Outdated Show resolved Hide resolved
docs/recipes/amp-alertmanager-terraform.md Outdated Show resolved Hide resolved
docs/recipes/amp-alertmanager-terraform.md Outdated Show resolved Hide resolved
docs/recipes/amp-alertmanager-terraform.md Outdated Show resolved Hide resolved
docs/recipes/amp-alertmanager-terraform.md Outdated Show resolved Hide resolved
cd ./sample-apps/prometheus
docker build . -t prometheus-sample-app:latest
```
2. Push this image to a registry such as Amazon ECR or DockerHub.
Copy link
Member

Choose a reason for hiding this comment

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

Let's focus on one, say, ECR and provide the commands. You can reuse, for example, https://aws-observability.github.io/aws-o11y-recipes/recipes/fargate-eks-metrics-go-adot-ampamg/#build-container-image


### Deploying the Terraform module to configure Amazon Managed service for Prometheus workspace, recording rules & alert manager

Now, we will provision a Amazon Managed service for Prometheus workspace and will define an alerting rule that causes the Alert Manager to send a notification if a certain condition (defined in expr) holds true for a specified time period (for). Code in the Terraform language is stored in plain text files with the .tf file extension. There is also a JSON-based variant of the language that is named with the .tf.json file extension.
Copy link
Member

Choose a reason for hiding this comment

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

service -> Service

Copy link
Member

Choose a reason for hiding this comment

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

Turn code fragments such expr and for into code using expr and for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

}
```

Once the file is created, execute the below commands to provision the resource using terraform:
Copy link
Member

Choose a reason for hiding this comment

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

Turn above file into a proper resource (supporting file as per https://github.com/aws-observability/aws-o11y-recipes/#writing-recipes) and make sure to clean it up. It looks like there's an extra EOF in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have created a file main.tf as per the direction

terraform apply
```

Once the above steps are complete, verify the setup end-to-end by using awscurl and query the endpoint. Look for the metric “metric:recording_rule”, and, if you successfully find the metric, then you’ve successfully created a recording rule:
Copy link
Member

Choose a reason for hiding this comment

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

If you are using awscurl here you need to add it to the pre-reqs section as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the pre-req

awscurl https://aps-workspaces.us-east-1.amazonaws.com/workspaces/$WORKSPACE_ID/alertmanager/api/v2/alerts --service="aps" -H "Content-Type: application/json"
```

## Clean up
Copy link
Member

Choose a reason for hiding this comment

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

Remove this section, it's a dupe (see below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleared the section

@mhausenblas
Copy link
Member

OK, thanks for addressing the issues @Saaish but there are still some things to be done before we can merge it:

  • In amp.md you're linking to prometheus-adapter-custom-metrics-autoscaling.md where it should be amp-alertmanager-terraform.md
  • You need to locally preview the rendering and you will then notice that a number of things are not properly formatted in Markdown, for example the pre-req list or the flags.
  • Please make sure that all variables and files are properly formatted, for example SNS_TOPIC not SNS_TOPIC.
  • For main.tf you need to call out all env vars that customers need to replace.
  • Split the awscurl commands and show the output.
  • The section headings should be short, ideally fitting in one line in the right-hand site ToC, for example, rather than "Deploying the Terraform module to configure Amazon Managed service for Prometheus workspace, recording rules & alert manager" say "Configure workspace with Terraform".

@awsvikram
Copy link
Contributor Author

OK, thanks for addressing the issues @Saaish but there are still some things to be done before we can merge it:

  • In amp.md you're linking to prometheus-adapter-custom-metrics-autoscaling.md where it should be amp-alertmanager-terraform.md
  • You need to locally preview the rendering and you will then notice that a number of things are not properly formatted in Markdown, for example the pre-req list or the flags.
  • Please make sure that all variables and files are properly formatted, for example SNS_TOPIC not SNS_TOPIC.
  • For main.tf you need to call out all env vars that customers need to replace.
  • Split the awscurl commands and show the output.
  • The section headings should be short, ideally fitting in one line in the right-hand site ToC, for example, rather than "Deploying the Terraform module to configure Amazon Managed service for Prometheus workspace, recording rules & alert manager" say "Configure workspace with Terraform".

Thanks for the feedback @mhausenblas . I incorporated the changes and did a local preview. Things looked good. Please do the needful

@mhausenblas
Copy link
Member

@Saaish thanks and there are still a number of things not addressed. From the flags (not a list, not code, see my previous comment) to the region env variable in main.tf you're not calling out to the inline comments above. At this point it might be faster if I merge it and take care of the edits myself rather than continue to play ping-pong here, lemme know what you prefer.

@mhausenblas mhausenblas merged commit 33c37ae into aws-observability:main Jan 24, 2022
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.

None yet

2 participants