Skip to content

feat: support memory profiling by adding ProfileType#565

Draft
xavilien wants to merge 4 commits into
grafana:mainfrom
semgrep:xavier/pyro-caml
Draft

feat: support memory profiling by adding ProfileType#565
xavilien wants to merge 4 commits into
grafana:mainfrom
semgrep:xavier/pyro-caml

Conversation

@xavilien

Copy link
Copy Markdown
Contributor

Currently, only CPU is supported in this SDK. This PR adds support for memory profiling by adding a new ProfileType to PyroscopeConfig, which is used by pprof::encode to configure the report sent to the pyroscope server. Closes #564.

Comment thread src/encode/pprof.rs
let (type_name, unit_name) = profiling_type.value_type();
let st = b.add_string(&type_name.to_string());
let unit = b.add_string(&unit_name.to_string());
b.profile.sample_type.push(ValueType { r#type: st, unit });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

memory profile code does not use PprofBuilder - it is encoded to pprof in another crate and we just transfer it with ReportData::RawPprof

Comment thread src/pyroscope.rs
pub tenant_id: Option<String>,
pub http_headers: HashMap<String, String>,
pub upload_interval: Duration,
pub profiling_type: ProfilingType,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure it should be a config option. I would expect it to be provided by the backend or profiling subssytem. We pass ReportBatch.profile_type now.

@korniltsev-grafanista

Copy link
Copy Markdown
Contributor

I dont' understand the problem this PR is trying to solve. Can you explain a bit more?

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.

Add support for memory profiling

2 participants