Skip to content

Config for pprof block and mutex#3655

Merged
cthulhu-rider merged 2 commits intomasterfrom
feature/block-mutex-pprof-cfg
Oct 31, 2025
Merged

Config for pprof block and mutex#3655
cthulhu-rider merged 2 commits intomasterfrom
feature/block-mutex-pprof-cfg

Conversation

@cthulhu-rider
Copy link
Contributor

@cthulhu-rider cthulhu-rider commented Oct 28, 2025

No description provided.

@cthulhu-rider cthulhu-rider force-pushed the feature/block-mutex-pprof-cfg branch from aeb263f to 11ed3e3 Compare October 28, 2025 09:30
@cthulhu-rider cthulhu-rider added neofs-storage Storage node application issues config Configuration format update or breaking change feature Completely new functionality labels Oct 28, 2025
@cthulhu-rider cthulhu-rider force-pushed the feature/block-mutex-pprof-cfg branch from 11ed3e3 to 725ec06 Compare October 28, 2025 09:33
@cthulhu-rider cthulhu-rider marked this pull request as ready for review October 28, 2025 09:39
@codecov
Copy link

codecov bot commented Oct 28, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 21.42857% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.40%. Comparing base (3c2b016) to head (2da0fd4).
⚠️ Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
cmd/neofs-node/pprof.go 0.00% 10 Missing ⚠️
cmd/neofs-node/config.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3655      +/-   ##
==========================================
+ Coverage   27.36%   27.40%   +0.04%     
==========================================
  Files         656      657       +1     
  Lines       41176    41207      +31     
==========================================
+ Hits        11268    11294      +26     
- Misses      28851    28859       +8     
+ Partials     1057     1054       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

End-rey
End-rey reviewed Oct 30, 2025
View reviewed changes
cmd/neofs-node/config/pprof/config.go Outdated
// Normalize normalizes x fields.
func (x *Pprof) Normalize() {
if x.Address == "" {
x.Address = "localhost:6060"
Copy link
Contributor

@End-rey End-rey Oct 30, 2025

Choose a reason for hiding this comment

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

Create a default variable to use in the test, as we do in other config packages.

Copy link
Contributor Author

@cthulhu-rider cthulhu-rider Oct 31, 2025

Choose a reason for hiding this comment

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

i dont think test should use these variable. Instead, it should assert exact value

Copy link
Contributor

@End-rey End-rey Oct 31, 2025

Choose a reason for hiding this comment

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

I meant the default value constants.

Copy link
Contributor Author

@cthulhu-rider cthulhu-rider Oct 31, 2025

Choose a reason for hiding this comment

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

i know. Having this

neofs-node/cmd/neofs-node/config/service/profiler_test.go

Line 17 in 67e9ba2

require.Equal(t, serviceconfig.ProfilerAddressDefault, emptyConfig.Pprof.Address)

asserts that config defaults to some variable. Instead, i wanna assert exact value

Copy link
Member

@carpawell carpawell Oct 31, 2025

Choose a reason for hiding this comment

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

as we do in other config packages

i am always for this approach, we either agree with what we do or we need to change it everywhere if we think that it is wrong (or an issue)

asserts that config defaults to some variable

imo, that is what is needed: package export some variable that says what value is used if no config values are found, and test asserts it. what bothers you?

Copy link
Contributor Author

@cthulhu-rider cthulhu-rider Oct 31, 2025

Choose a reason for hiding this comment

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

what bothers you?

if i change var value, nothing breaks. Although it should

carpawell
carpawell reviewed Oct 30, 2025
View reviewed changes
@cthulhu-rider cthulhu-rider force-pushed the feature/block-mutex-pprof-cfg branch from 725ec06 to 1927a55 Compare October 31, 2025 10:52
@cthulhu-rider
sn/config: Provide way to enable block and mutex profiling
12723de 
There was no way to enable these profiles before.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider cthulhu-rider force-pushed the feature/block-mutex-pprof-cfg branch from 1927a55 to 12723de Compare October 31, 2025 10:58
@cthulhu-rider
sn/config: Use non-default metrics and pprof endpoints for testing
2da0fd4 
To distinguish between default and configuration

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider cthulhu-rider merged commit 51eb8ab into master Oct 31, 2025
19 of 21 checks passed
@cthulhu-rider cthulhu-rider deleted the feature/block-mutex-pprof-cfg branch October 31, 2025 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@End-rey End-rey End-rey left review comments

@carpawell carpawell carpawell approved these changes

@roman-khimov roman-khimov Awaiting requested review from roman-khimov roman-khimov is a code owner

Assignees

No one assigned

Labels

config Configuration format update or breaking change feature Completely new functionality neofs-storage Storage node application issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cthulhu-rider @carpawell @End-rey