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

@datadog/pprof integration #39

Closed
wants to merge 41 commits into from

Conversation

notaphplover
Copy link
Contributor

This is, for now, a work in progress that aims to update the library in such a way it uses the @datadog/pprof library for profiling purposes.

An overview of the changes is the folowing one:

  • PyroscopeConfig has been updated with equivalent options.
  • A PyroscopeProfiler has been added internally to encapsulate different API operations.
  • Pyroscope integration is now enhanced passing from and until parameters to the ingest endpoint.

@CLAassistant
Copy link

CLAassistant commented Nov 16, 2023

CLA assistant check
All committers have signed the CLA.

import type perftools from 'pprof/proto/profile'
import debug from 'debug'
import axios, { AxiosBasicCredentials, AxiosError } from 'axios'
import FormData from 'form-data'
import 'regenerator-runtime/runtime'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we import this? Users of this library should decide whether or not they want to import this runtime, but users that do not rely on this runtime won't be interested in importing it

jest.config.js Outdated
Comment on lines 9 to 11
"./cpu.js": "<rootDir>/src/cpu.ts",
"./wall.js": "<rootDir>/src/wall.ts",
"./heap.js": "<rootDir>/src/heap.ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This belongs to the old PR and should be removed

@@ -29,7 +29,7 @@ Pyroscope supports two main operation modes:

Push mode means the package itself uploads profile data to a pyroscope server, when pull mode means you provide pyroscope server with an endponts to scrape profile data

NodeJS Pyroscope module supports collecting wall-time and heap. More details you may find [here](https://cloud.google.com/profiler/docs/concepts-profiling)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Docs must be updated

@@ -0,0 +1,21 @@
export interface PyroscopeConfig {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To do: add source map support

})
})
it('should respond to repetitive cpu calls', async () => {
const app = express()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@korniltsev we need to think about this case. What should we do on consecutive calls? How do we handle the profiler? The old implementation closed the profiler so it could not handle consecutive cpu calls

@korniltsev
Copy link
Collaborator

cc @grafakus , @Rperry2174

@korniltsev
Copy link
Collaborator

@notaphplover
Sorry for long wait. We will take a look at the PR after holidays.

@petethepig petethepig marked this pull request as ready for review March 27, 2024 23:58
@petethepig
Copy link
Member

Hey @notaphplover / @korniltsev — I finished this branch and merged it today.

I had to use the old PR though instead of this one because I could not push to this PR — sorry about the confusion.

And thanks for the contributions!

@petethepig petethepig closed this Apr 2, 2024
@korniltsev
Copy link
Collaborator

🎉🎉🎉

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.

5 participants