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

RFC 400: RUM AppMonitor L2 Construct #408

Closed
wants to merge 22 commits into from

Conversation

WinterYukky
Copy link
Contributor

@WinterYukky WinterYukky commented Feb 16, 2022

This is a request for comments about RUM AppMonitor L2 Construct. See #400 for
additional details.


By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license

@madeline-k
Copy link
Contributor

Thanks for opening this Pull Request, @WinterYukky! I have not had a chance to review the APIs yet, so this is not signed off yet. I plan to review this later this week.

@WinterYukky
Copy link
Contributor Author

Thank you @madeline-k for your quick reply. Please take your time🕊️

Copy link
Contributor

@madeline-k madeline-k left a comment

Choose a reason for hiding this comment

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

This is a great start @WinterYukky! I think for next steps, you should open a PR to aws-cdk with the minimum feature set. Then we can see it in action as an experimental L2 before finalizing this RFC.

The minimum feature set here should just be the AppMonitor construct with default behavior of creating the Identity Pool for you. We can add on other features and other methods of authorization in an iterative manner.

text/0400-appmonitor-l2.md Outdated Show resolved Hide resolved
text/0400-appmonitor-l2.md Outdated Show resolved Hide resolved
text/0400-appmonitor-l2.md Outdated Show resolved Hide resolved
CloudWatch RUM is monitoring tool that can perform real user monitoring to collect
and view client-side data about your web application performance from actual user sessions in near real time.

This module supports the ability for users to create CloudWatch RUM and retrieve code snippets on CloudFormation.
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is not clear to someone who doesn't know that RUM requires the user to paste a code snippet into each page of their application for RUM to be able to collect client side data.

For the intro, I'd recommend copy-pasting some lines from the official RUM docs, and also linking back to them.

text/0400-appmonitor-l2.md Outdated Show resolved Hide resolved

### Code Snippet

AppMonitor generates a code snippet that looks like to create on the management console. Note, however,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this sentence is missing something?

});

// (function(n,i,v,r,s,c,x,z){...})
const codeSnippet = appMonitor.generateCodeSnippet('CodeSnippet');
Copy link
Contributor

Choose a reason for hiding this comment

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

How would the customer then use the code snippet? We should show an example of how they can take this an actually insert it into their application code.

text/0400-appmonitor-l2.md Outdated Show resolved Hide resolved
Comment on lines 158 to 168
const html = s3deploy.Source.data('index.html', `<html>
<head>
<script src="/rum.js" async="true"></script>
</head>
<body>Hello RUM</body>
</html>`);

new s3deploy.BucketDeployment(this, 'BucketDeployment', {
sources: [html, rumJs],
destinationBucket: webSiteBucket
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Practically speaking, I don't think developers are going to write raw HTML inline like this. Also part of this is boilerplate, like specifying the script name rum.js twice. Can we make this simpler? And closer to a real-world scenario?

text/0400-appmonitor-l2.md Outdated Show resolved Hide resolved
WinterYukky and others added 7 commits March 24, 2022 00:38
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
@mergify mergify bot dismissed madeline-k’s stale review March 23, 2022 16:27

Pull request has been modified.

@WinterYukky
Copy link
Contributor Author

Hello @madeline-k.
I fixed RFC and created the PR aws/aws-cdk#18364 with the minimum feature set .
Cloud you review the PR?

@evgenyka evgenyka added the l2-request request for new L2 construct label Aug 10, 2023
@mrgrain mrgrain closed this Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
l2-request request for new L2 construct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants