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

Pyroscope Integration #5243

Merged
merged 11 commits into from
Mar 8, 2024
Merged

Pyroscope Integration #5243

merged 11 commits into from
Mar 8, 2024

Conversation

Cliftonz
Copy link
Contributor

What change does this PR introduce?

This PR introduces the integration of Pyroscope, an open-source continuous profiling platform, into the Novu codebase. By incorporating Pyroscope, we enable real-time performance monitoring and profiling of our Node.js application. This integration involves adding the Pyroscope Node.js agent to our dependencies and initializing it within our application's entry point to begin automatic profiling.

Why was this change needed?

As Novu continues to grow and scale, ensuring optimal performance and resource utilization becomes increasingly critical. The integration of Pyroscope provides us with a powerful tool for continuous profiling, allowing us to identify performance bottlenecks, optimize code, and enhance the overall efficiency and reliability of Novu. This proactive approach to performance monitoring ensures that we can maintain a high-quality service for our users, even as the complexity and scale of our operations increase.

By implementing continuous profiling with Pyroscope, we gain deeper insights into the runtime behavior of our application, enabling us to make data-driven decisions for performance improvements. This change is a step forward in our commitment to delivering a robust and high-performing notification system.

This commit introduces a profiling service using Pyroscope in the `application-generic` package. Several environment variables for configuring the profiling service are added, and the relevant dependencies are included in the package.json file. The new service enables and initializes Pyroscope if it's enabled, and it is also exported in the index file of the package.
This update integrates the ProfilingModule from the '@novu/application-generic' package into the 'webhook', 'ws', 'api', and 'worker' applications. This new incorporation allows for enhanced performance tracking across various modules. The commit also adjusts the 'packageJson' import statement to a more efficient syntax using 'import * as'.
This commit introduces a new profiling controller in the application-generic package. The controller contains methods to profile CPU and Heap data using the NestJS framework. Error handling also added in case there is an issue with collecting profiling data.
The profiling service and module have been enhanced for extended profiling tasks. A new profiling controller with methods for CPU and Heap data profiling has been added. Furthermore, improvements have been made for logging, error handling, and the existing initialization process. Configuration variables have also been renamed for clarity.
This commit adds a new docker-compose.monitoring.yml file for the development environment. It includes configurations for LocalStack, MongoDB, Redis, and Pyroscope services, each with their respective health checks. This will allow us to manage these services more easily and improve monitoring during local development.
The commit removes the unused Logger import and a console.log statement from profiling.service.ts in the application-generic package. This helps to clean up redundant code and make the module more concise.
Instead of using an imported instance of PinoLogger, the NestJS Logger has been directly used for logging in the profiling.service.ts. This change streamlines the code by making use of the built-in NestJS utilities, removing an unnecessary dependency.
@Cliftonz Cliftonz requested review from LetItRock and rifont February 28, 2024 19:55
Copy link

netlify bot commented Feb 28, 2024

Deploy Preview for dev-web-novu ready!

Name Link
🔨 Latest commit a02f489
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/65e9f857d5fe29000885c283
😎 Deploy Preview https://deploy-preview-5243--dev-web-novu.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Cliftonz added 4 commits March 7, 2024 10:17
# Conflicts:
#	pnpm-lock.yaml
Package versions in pnpm-lock.yaml have been updated to '^0.24.0' from the old version '^0.23.1'. All instances of the old version have been replaced across numerous dependencies and development dependencies. This ensures that all packages use the latest stable version, maintaining consistency and reducing possible conflicts or issues due to version mismatch.
…he provided code diffs, here's the correct commit message:

Add new words to cspell.json

New words "pyroscope", "HEAY", "Pyroscope", "PYROSCOPE" have been added to the dictionary in .cspell.json. This expands our dictionary to reduce false positives during spell checking and helps ensure accurate and consistent spelling across the project.
Comment on lines +6 to +7
@Controller('/debug/pprof')
@ApiExcludeController()
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess these won't be accessible from the outside right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, I have not hooked them to the api yet as with AWS ALB it is not able to block these request but istio can.
I can also have this set up where it is conditionally enabled.

@Cliftonz Cliftonz merged commit 4beba66 into next Mar 8, 2024
30 of 31 checks passed
@Cliftonz Cliftonz deleted the pyroscope_integration branch March 8, 2024 15:58
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.

2 participants