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

Add frame count to pprof message attributes #217

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Mar 24, 2023

Profiling team requests that pprof messages should be accompanied by the number of stack frames (non-unique) represented in the pprof.
@gsmirnov-splk please review

@laurit laurit requested review from a team as code owners March 24, 2023 08:35
@Kielek
Copy link
Contributor

Kielek commented Mar 24, 2023

What's about already released implementation without this parameter? Will it be handled correctly?

@gsmirnov-splk
Copy link

LGTM, thanks

@gsmirnov-splk
Copy link

What's about already released implementation without this parameter? Will it be handled correctly?

@Kielek yes, we will still support agents that do not report this attribute. No hard date on how long, but my gut feeling suggests that at least for a year.

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

@gsmirnov-splk, I suppose you should support as long as we support any already released library.
Breaking it could break environment for our customers.
I am fine with adding new attribute as long as you agreed deprecation date with PMs.

@pellared
Copy link
Contributor

pellared commented Mar 24, 2023

@Kielek please let us know if you want this PR to be merged after the 1.5.0 release (#193)

@pellared
Copy link
Contributor

@seemk Can you please take a look as well?

@akubik-splunk
Copy link

Per discussion with @gsmirnov-splk the messages without support will continue to be supported by backend for at lest 12 months from now. Additional recommendations for release tagging will be made to indicate releases deprecated and no longer supported

@seemk
Copy link
Contributor

seemk commented Mar 24, 2023

Node.js already does sample coalescing, i.e. in memory profiling we multiply the sample count with allocation size per sample. Does this mean the total number reported here should be the amount of samples before this multiplication?

@laurit
Copy link
Contributor Author

laurit commented Mar 24, 2023

Node.js already does sample coalescing, i.e. in memory profiling we multiply the sample count with allocation size per sample. Does this mean the total number reported here should be the amount of samples before this multiplication?

My understanding is that this is used to estimate pprof data size, so it should be the number after coalescing.

@gsmirnov-splk
Copy link

@seemk The backend limiting factor is the number of (non-unique) frames coming from all the stack traces. If you coalesce 10 stack traces into one (such that when we read the pprof, we read one sample instead of 10), then this attribute should be after coalescing. If it is still 10 sample-s recorded in the pprof, then it should be before the coalescing.

@seemk
Copy link
Contributor

seemk commented Mar 24, 2023

@seemk The backend limiting factor is the number of (non-unique) frames coming from all the stack traces. If you coalesce 10 stack traces into one (such that when we read the pprof, we read one sample instead of 10), then this attribute should be after coalescing. If it is still 10 sample-s recorded in the pprof, then it should be before the coalescing.

Thanks!

@breedx-splk breedx-splk merged commit 03f5a48 into signalfx:main Mar 27, 2023
@laurit laurit deleted the pprof-frame-count branch March 29, 2023 21:48
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.

9 participants