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

feat: initial pyroscope integration #1006

Merged
merged 12 commits into from
Aug 3, 2023

Conversation

evan-forbes
Copy link
Member

Description

leaving as a draft and WIP until I test this out more. not ready for review

closes #1004

@evan-forbes evan-forbes self-assigned this May 7, 2023
@evan-forbes evan-forbes changed the title feat: initial pyroscope integration [WIP] feat: initial pyroscope integration May 7, 2023
@evan-forbes evan-forbes changed the title [WIP] feat: initial pyroscope integration feat: initial pyroscope integration Aug 2, 2023
@evan-forbes
Copy link
Member Author

Here's a pic of this running on one of the mocha validators
Screenshot from 2023-08-02 16-50-11

@evan-forbes evan-forbes marked this pull request as ready for review August 2, 2023 14:57
@evan-forbes
Copy link
Member Author

why the linter is complaining about

//nolint:staticcheck // SA1019 Existing use of deprecated but supported dial option.

now is beyond me.

Comment on lines -91 to +92
//nolint:staticcheck // SA1019 Existing use of deprecated but supported dial option.
conn, err := grpc.Dial(cli.addr, grpc.WithInsecure(), grpc.WithContextDialer(dialerFunc))
conn, err := grpc.Dial(cli.addr, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithContextDialer(dialerFunc))
Copy link
Member Author

Choose a reason for hiding this comment

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

I do not know why the linter started complaining for this here, but this seemed like the best solution

Copy link
Collaborator

Choose a reason for hiding this comment

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

seems resolved by 310cd2a

rach-id
rach-id previously approved these changes Aug 2, 2023
Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

utAck

rootulp
rootulp previously approved these changes Aug 2, 2023
Comment on lines -91 to +92
//nolint:staticcheck // SA1019 Existing use of deprecated but supported dial option.
conn, err := grpc.Dial(cli.addr, grpc.WithInsecure(), grpc.WithContextDialer(dialerFunc))
conn, err := grpc.Dial(cli.addr, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithContextDialer(dialerFunc))
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems resolved by 310cd2a

pkg/trace/flags.go Outdated Show resolved Hide resolved
test/e2e/pkg/infrastructure.go Outdated Show resolved Hide resolved
@evan-forbes evan-forbes dismissed stale reviews from rootulp and rach-id via 95f26b7 August 3, 2023 09:52
staheri14
staheri14 previously approved these changes Aug 3, 2023
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

LGTM!

node/tracing.go Outdated Show resolved Hide resolved
rootulp
rootulp previously approved these changes Aug 3, 2023
…elestia-core into evan/pyroscope-integration
@evan-forbes evan-forbes dismissed stale reviews from rootulp and staheri14 via da83f3e August 3, 2023 13:47
@evan-forbes
Copy link
Member Author

ok apologies, had a last minute addition after the first round of reviews where I added the ability to specify which profiles to run in the config, along with changing the defaults see c9794fa

its identical to the rest of the additions to the config

@evan-forbes evan-forbes merged commit 518e0e1 into v0.34.x-celestia Aug 3, 2023
17 checks passed
@evan-forbes evan-forbes deleted the evan/pyroscope-integration branch August 3, 2023 16:16
@cmwaters
Copy link
Contributor

@evan-forbes can you add some documentation here. I would like to be able to run this

evan-forbes added a commit that referenced this pull request Sep 28, 2023
leaving as a draft and WIP until I test this out more. not ready for
review

closes #1004

---------

Co-authored-by: Rootul P <[email protected]>
@faddat faddat mentioned this pull request Feb 22, 2024
3 tasks
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.

Integrate easy to digest remote continuous profiling
5 participants