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

Update CloudFormation template to create log group & stream on deploy #51962

Merged
merged 5 commits into from
May 18, 2023

Conversation

sanchitmalhotra126
Copy link
Contributor

@sanchitmalhotra126 sanchitmalhotra126 commented May 18, 2023

Updates the main cloudformation template with a step to create a browser events Log Group and Log Stream for the given stack. Some implementation notes:

  • Ideally, the rails application would be able to receive variables like the log group/stream names directly from the Cloudformation template; however we don't currently have a good way to do that, so we are instead calculating the name in a predictable way, in both the template and the Rails controller independently
  • In order to satisfy existing role permissions, the Log Group name needs to be prefixed with the environment name. However, we also want the log group & stream to be unique per adhoc instance, so adhocs are not all writing to the same group. To do this I needed to add some conditional logic that uses the stack_name if on an adhoc (which is the full "adhoc-my-adhoc-name") or environment otherwise (which is "production", "test", etc). We can't just use stack_name because on production, this returns autoscale-prod which isn't as clear and wouldn't satisfy the constraint of the log group name needing to be prefixed with the environment name.
    • On adhocs, the log group will look like "adhoc-my-branch-name-browser-events" and the log stream will look like "adhoc-my-branch-name".
    • On other environments, the log group should look like "production-browser-events" and the log stream should just be "production"
  • Originally, we were planning on using the domain name as a unique identifier. However this posed two issues - due to the existing role permissions, we'd still have to prefix with the environment name (or add a new resource name to the allow list), and on adhocs, the template uses code.org as the BaseDomainName, which doesn't match what the Rails controller calculates as the base domain, which is cdn-code.org. We could use the <%=domain> value inside the template to get cdn-code.org but this felt unnecessarily verbose given we were already prefixing the name with the environment.

Links

https://codedotorg.atlassian.net/browse/SL-799

Testing story

Tested on an adhoc: https://adhoc-browser-events-template-test-studio.cdn-code.org/
Generated Log Group: https://us-east-1.console.aws.amazon.com/cloudwatch/home?region=us-east-1#logsV2:log-groups/log-group/adhoc-browser-events-template-test-browser-events
Generated Log Stream: https://us-east-1.console.aws.amazon.com/cloudwatch/home?region=us-east-1#logsV2:log-groups/log-group/adhoc-browser-events-template-test-browser-events/log-events/adhoc-browser-events-template-test

@sanchitmalhotra126 sanchitmalhotra126 requested review from a team May 18, 2023 00:25
@@ -617,7 +617,7 @@ Resources:
<%= component 'alarms'%>
<% end -%>


<%= component 'browser_event_logging'%>
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be handy to rename this to logging and then Infrastructure team will add other logging Resources to this component in the future that are currently configured manually, provisioned in the top level template, or in some standalone template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

@sureshc sureshc left a comment

Choose a reason for hiding this comment

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

This looks great! Sorry the stack/domain naming turned out to be a hassle.

@sanchitmalhotra126 sanchitmalhotra126 merged commit df96628 into staging May 18, 2023
2 checks passed
@sanchitmalhotra126 sanchitmalhotra126 deleted the sanchit/metrics/cloud-formation-template branch May 18, 2023 23:11
pablo-code-org pushed a commit that referenced this pull request May 25, 2023
…#51962)

* Test adding browser event log groups and stream to template

* Try prefixing with env name

* Try out <domain>

* try stack name as default on adhoc

* Rename component to logging
snickell pushed a commit that referenced this pull request Feb 3, 2024
…#51962)

* Test adding browser event log groups and stream to template

* Try prefixing with env name

* Try out <domain>

* try stack name as default on adhoc

* Rename component to logging
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.

None yet

2 participants