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

[Bug] Fix pprof endpoint #505

Merged
merged 4 commits into from
Apr 26, 2024
Merged

[Bug] Fix pprof endpoint #505

merged 4 commits into from
Apr 26, 2024

Conversation

okdas
Copy link
Member

@okdas okdas commented Apr 26, 2024

Summary

Pprof prevents services from starting. It was not run inside a routine and caused a lock where it shouldn't have been.

Issue

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

Documentation changes (only if making doc changes)

  • make docusaurus_start; only needed if you make doc changes

Local Testing (only if making code changes)

  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • See quickstart guide for instructions

PR Testing (only if making code changes)

  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.
    • THIS IS VERY EXPENSIVE, so only do it after all the reviews are complete.
    • Optionally run make trigger_ci if you want to re-trigger tests without any code changes
    • If tests fail, try re-running failed tests only using the GitHub UI as shown here

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

@okdas okdas added the bug Something isn't working label Apr 26, 2024
@okdas okdas added this to the Shannon Private TestNet milestone Apr 26, 2024
@okdas okdas requested review from red-0ne and Olshansk April 26, 2024 18:47
@okdas okdas self-assigned this Apr 26, 2024
@okdas okdas changed the title [Bug] Fix pprof [Bug] Fix pprof endpoint Apr 26, 2024
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

A small change request w.r.t server shutdown.

Would also be great if we have this server creation pattern done with metrics.

go func() {
<-ctx.Done()
app.logger.Info().Str("endpoint", addr).Msg("stopping a pprof endpoint")
server.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using server.Shutdown(ctx) which does graceful shut down?

I would also have the same pattern for metrics server, instead of relying on tcp.Listener

	server := &http.Server{
		Addr:    addr,
		Handler: pprofMux,
	}
	...
	server.ListenAndServe()
	...
	server.Shutdown(ctx)

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'll do this! I wasn't sure which one to use. I guess I should've asked gpt first.

Screenshot 2024-04-26 at 12 37 11 PM

@okdas okdas requested a review from red-0ne April 26, 2024 19:44
@okdas okdas merged commit b96f7bb into main Apr 26, 2024
9 checks passed
@okdas okdas deleted the dk-fix-pprof branch April 26, 2024 19:58
bryanchriswhite added a commit that referenced this pull request Apr 29, 2024
…cept

* pokt/main:
  [Dashboard] Supplier logs dashboard (#502)
  [Utility] Update SessionSettlement when app goes into a negative amount (#509)
  [Testing] Supplier staking E2E test (#498)
  [RelayMiner] Support https backend urls (#507)
  [Bug] Fix pprof endpoint (#505)
  [LocalNet] Remove tilt-restart-wrapper (#504)
  [LocalNet][Hotfix] Turn off pproff by default (#500)
  [Tooling] Add pprof endpoints and documentation (#484)
  [Documentation] Basic blockchain operations (#454)
  [BlockClient] Refactor BlockClient to fetch latest block at init (#444)
okdas added a commit that referenced this pull request Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants