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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add monitoring to the docker image #1240

Merged
merged 1 commit into from
Jun 25, 2021

Conversation

mathieu-pousse
Copy link
Contributor

@mathieu-pousse mathieu-pousse commented Jun 15, 2021

As we are now using this docker image in prod, we needed to add some monitoring to understand the overall performances of the project.

Here is the result of what is running in prod for us.

We are able to produce such dashboard to ensure everything is running fine and set alerting on it.
This also helps us to see the most time consuming transformation was the SeparateKeyframes that was useless for us and lead me to the previous MR 馃槃

image

BTW, as we are running in k8s, the fact of having metrics exposed will help us to configure an HorizontalPodAutoscaler based on those metrics.

My main concern is about the refactoring I did of the profiling in the project and the way to write the doProfile method. Let me know.

Hope it helps

Copy link
Collaborator

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

My main concern is about the refactoring I did of the profiling in the project and the way to write the doProfile method. Let me know.

Your implementation LGTM, much nicer than before.

@samouri has to review the docker integration

Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

@mathieu-pousse, thank you for the contribution! This looks great. I'll review it later today

Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

Some small nits and comments. Overall this looks fantastic :)

packages/optimizer-docker/README.md Outdated Show resolved Hide resolved
packages/optimizer-docker/README.md Outdated Show resolved Hide resolved
packages/optimizer-docker/README.md Outdated Show resolved Hide resolved
packages/optimizer-docker/index.js Outdated Show resolved Hide resolved
packages/optimizer-docker/index.js Outdated Show resolved Hide resolved
packages/optimizer-docker/index.js Outdated Show resolved Hide resolved
packages/optimizer-docker/metrics.js Outdated Show resolved Hide resolved
packages/optimizer/lib/DomTransformer.js Outdated Show resolved Hide resolved
packages/optimizer/lib/DomTransformer.js Outdated Show resolved Hide resolved
@mathieu-pousse
Copy link
Contributor Author

I pushed the fixes

I am wondering how does the CI build the image.

Will it firstly build the new version of @ampproject/toolbox-optimizer then build the image that will pull the freshly created new bundle ?

@samouri
Copy link
Member

samouri commented Jun 18, 2021

I am wondering how does the CI build the image.

We actually don't have a good system for this yet.
Right now we manually build + push new images.
I'll create an issue to integrate image build/release with CD.

@mathieu-pousse
Copy link
Contributor Author

mathieu-pousse commented Jun 19, 2021

Did I broke the CI with my PR ? Let me know

@sebastianbenz
Copy link
Collaborator

Noticed one problem that was causing the optimizer tests to fail.

@sebastianbenz
Copy link
Collaborator

And there seems to be another problem that jest fails to init optimizer-docker/metrics.js. One simple fix is to ignore the optimizer-docker dir during tests by adding this to jest.config.js:

   // An array of regexp pattern strings that are matched against all test paths, matched tests are skipped
-  // testPathIgnorePatterns: [
-  //   "/node_modules/"
-  // ],
+   testPathIgnorePatterns: [
+     "/node_modules/",
+     "<rootDir>/packages/optimizer-docker"
+   ],

@mathieu-pousse
Copy link
Contributor Author

馃帀

Copy link
Collaborator

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

Awesome - thanks!

@sebastianbenz sebastianbenz merged commit 1d2a067 into ampproject:main Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants