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

fix: increase stack max depth at 128 #159

Merged
merged 1 commit into from Aug 22, 2022

Conversation

viglia
Copy link
Contributor

@viglia viglia commented Aug 18, 2022

At the moment MAX_DEPTH is set to 32.

This is a bit low as a value and in some cases important frames are cut out of the final result.

@viglia
Copy link
Contributor Author

viglia commented Aug 18, 2022

@YangKeao if this looks good to you and it's merged, could you please create a new release after?

Thank you

@viglia viglia changed the title fix: increate stack max depth at 128 fix: increase stack max depth at 128 Aug 18, 2022
Copy link
Member

@YangKeao YangKeao left a comment

Choose a reason for hiding this comment

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

LGTM

@YangKeao
Copy link
Member

I'd fix some clippy before merge this PR 😸 .

@viglia
Copy link
Contributor Author

viglia commented Aug 19, 2022

@YangKeao thanks for taking such a quick look at this 😀

I'd fix some clippy before merge this PR 😸 .

I'm a bit confused about this. Do you mean making clippy less strict about warnings or fixing the warnings themselves?

I see most of the warnings come from the prost create but in that repo the clippy job was totally removed from CI (link) so each time some clippy-unfriendly syntax is merged there it'll have repercussion here in pprof.

I can only see a few warnings coming from pprof.

And sometimes those warnings seems very stricts (see example below) and I'm not sure if they should be treated as an error

error: very complex type used. Consider factoring parts into `type` definitions
Error:   --> src/report.rs:34:28
   |
34 |     frames_post_processor: Option<Box<dyn Fn(&mut Frames)>>,
   |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@YangKeao
Copy link
Member

I'm a bit confused about this. Do you mean making clippy less strict about warnings or fixing the warnings themselves?

See #160 . I fixed some clippy warnings themselves, and allow all inside the prost mod.

@Swatinem
Copy link

I think you can just #[allow(...)] a bunch of clippy rules. Suggesting/forcing you to use a type alias for an Option<Box<dyn Fn>> is a bit too annoying I would argue.

Copy link
Member

@YangKeao YangKeao left a comment

Choose a reason for hiding this comment

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

LGTM

@YangKeao
Copy link
Member

YangKeao commented Aug 22, 2022

@viglia Please fix the DCO (by git commit --amend -s), and rebase with the newest master (which fixes the clippy), then we can merge this PR 😃

Signed-off-by: Francesco Vigliaturo <francesco.vigliaturo@sentry.io>
@YangKeao YangKeao merged commit 84109e9 into tikv:master Aug 22, 2022
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.

None yet

3 participants