Fix/profiler v31 #9591
Fix/profiler v31 #9591
Conversation
(cherry picked from commit 9b6aafb) Co-authored-by: Dev Ojha <[email protected]>
Signed-off-by: PaddyMc <[email protected]>
WalkthroughAdds DataDog profiler startup to the app initialization, changes two fee globals to use cloned copies of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (1)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
x/txfees/keeper/mempool-1559/code.go (1)
34-37: Decoupling base‑fee globals fromConsensusMinFeeviaClone()looks good; update doc commentUsing
types.ConsensusMinFee.Clone()for bothDefaultBaseFeeandMinBaseFeeis a good change: it avoids aliasing the sharedConsensusMinFeevalue and keeps future mutations of these globals from accidentally affecting the constant.The block comment above still documents different initial numeric values for
DefaultBaseFeeandMinBaseFee, which no longer matches reality (both now start from the sameConsensusMinFee). It’d be good to update that comment (and any related docs) to reflect the actual defaults so operators aren’t misled.Also applies to: 49-50
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumgo.workis excluded by!**/*.work,!**/*.work
📒 Files selected for processing (2)
app/app.go(2 hunks)x/txfees/keeper/mempool-1559/code.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
x/txfees/keeper/mempool-1559/code.go (1)
x/txfees/types/constants.go (1)
ConsensusMinFee(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: e2e
- GitHub Check: go-split-test-files
- GitHub Check: osmosisd-darwin-arm64
- GitHub Check: osmosisd-darwin-amd64
- GitHub Check: osmosisd-linux-amd64
- GitHub Check: osmosisd-linux-arm64
- GitHub Check: test
- GitHub Check: check-proto
- GitHub Check: Run golangci-lint
- GitHub Check: Summary
| semconv "go.opentelemetry.io/otel/semconv/v1.4.0" | ||
|
|
||
| wasmtypes "github.com/CosmWasm/wasmd/x/wasm/types" | ||
| "github.com/DataDog/dd-trace-go/v2/profiler" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate and examine app/app.go
fd -type f -name "app.go" | head -20Repository: osmosis-labs/osmosis
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Locate app/app.go
find . -name "app.go" -path "*/app/*" | head -5Repository: osmosis-labs/osmosis
Length of output: 76
🏁 Script executed:
#!/bin/bash
# Get file info and read around the import line
wc -l ./app/app.go
echo "=== Lines 1-30 (import section) ==="
head -30 ./app/app.go | cat -nRepository: osmosis-labs/osmosis
Length of output: 1127
🏁 Script executed:
#!/bin/bash
# Read the NewOsmosisApp function area where profiler.Start is called
sed -n '280,310p' ./app/app.go | cat -nRepository: osmosis-labs/osmosis
Length of output: 1072
🏁 Script executed:
#!/bin/bash
# Search for profiler.Stop and any profiler config
echo "=== Searching for profiler.Stop() ==="
rg -n "profiler\.Stop" ./app/app.go
echo -e "\n=== Searching for profiler config/flag patterns ==="
rg -n -i "profiler.*config\|config.*profiler\|profiler.*flag" ./app/app.go
echo -e "\n=== Checking function signature of Start call context ==="
sed -n '200,230p' ./app/app.go | cat -nRepository: osmosis-labs/osmosis
Length of output: 1653
🏁 Script executed:
#!/bin/bash
# Find NewOsmosisApp function
echo "=== Finding NewOsmosisApp function ==="
rg -n "^func.*NewOsmosisApp" ./app/app.go
echo -e "\n=== Searching entire repo for profiler.Stop ==="
rg -r "profiler\.Stop" .
echo -e "\n=== Looking for main.go or cmd files ==="
find . -name "main.go" -o -name "cmd" -type d | head -10Repository: osmosis-labs/osmosis
Length of output: 50533
🏁 Script executed:
#!/bin/bash
# Search for profiler in Go files only
echo "=== Searching for profiler in Go files ==="
rg -n "profiler" --type=go ./app/app.go
echo -e "\n=== Looking for cmd/main.go ==="
find . -path "*/cmd/*" -name "main.go" | head -5
echo -e "\n=== Checking if Stop is called elsewhere in Go codebase ==="
rg -n "\.Stop\(\)" --type=go . | grep -i profiler | head -20Repository: osmosis-labs/osmosis
Length of output: 771
🏁 Script executed:
#!/bin/bash
# Read the full profiler block in app/app.go (lines 289-310)
echo "=== Full profiler block in app/app.go ==="
sed -n '289,310p' ./app/app.go | cat -n
echo -e "\n=== Check cmd/osmosisd/main.go for profiler Stop and shutdown ==="
grep -n "profiler" ./cmd/osmosisd/main.go
grep -n -i "shutdown\|deferred\|defer" ./cmd/osmosisd/main.go | head -10Repository: osmosis-labs/osmosis
Length of output: 1023
🏁 Script executed:
#!/bin/bash
# Read cmd/osmosisd/main.go to see structure
wc -l ./cmd/osmosisd/main.go
head -50 ./cmd/osmosisd/main.go | cat -nRepository: osmosis-labs/osmosis
Length of output: 642
🌐 Web query:
Datadog Go profiler BlockProfile MutexProfile GoroutineProfile default disabled
💡 Result:
Short answer: Datadog’s Go profiler enables only CPU and heap profiles by default; BlockProfile, MutexProfile and GoroutineProfile are disabled and must be explicitly enabled via profiler.WithProfileTypes (or equivalent config). [1][2]
Sources:
- Datadog Go profiler docs (shows Block/Mutex/Goroutine commented out / disabled by default). [1]
- Go runtime/pprof docs (notes block and mutex profiles are not enabled by default). [2]
🏁 Script executed:
#!/bin/bash
# Search for any config/flag handling related to profiler
echo "=== Search for config or flags related to profiler ==="
rg -n -i "config.*profiler\|profiler.*config\|profiler.*flag\|flag.*profiler" --type=go .
echo -e "\n=== Check if NewOsmosisApp takes any config parameter ==="
sed -n '258,270p' ./app/app.go | cat -nRepository: osmosis-labs/osmosis
Length of output: 592
Excellent. I have verified all the key claims in the review comment. Let me now generate the final rewritten review comment:
Make Datadog profiler startup best‑effort (don't panic) and reassess enabled profile types
The Datadog profiler starts unconditionally in NewOsmosisApp (line 290) and panics the entire process if startup fails. Since profiling is an optional observability feature, treating it as fatal makes node availability dependent on profiler configuration and environment (API key / agent / hostname, etc.).
Additionally, you're enabling all five profile types: CPU, Heap, Block, Mutex, and Goroutine. Datadog's documentation explicitly keeps Block, Mutex, and Goroutine disabled by default "to keep overhead low", and recommends enabling them only as needed. For a validator or full‑node, this overhead could be non‑trivial if turned on by default across the fleet.
Concrete suggestions:
- Handle
profiler.Starterrors as non‑fatal: log and continue instead ofpanic(line 304). - Optionally gate profiler startup behind a dedicated config flag (e.g., using
appOpts) so operators can enable it only where desired. - Reconsider whether Block/Mutex/Goroutine profiles should be enabled by default, or make those configurable.
- Ensure
profiler.Stop()is called once on clean shutdown (wire this in the daemon's entry point, likely via a defer or shutdown hook), so final profiles are flushed.
Example minimal change for error handling:
- // Initialize DataDog profiler if enabled
- err := profiler.Start(
+ // Initialize Datadog profiler (best-effort; do not fail app on error).
+ if err := profiler.Start(
profiler.WithService("osmosis-dd-profiler"),
@@
- )
- if err != nil {
- panic(err)
- }
+ ); err != nil {
+ logger.Error("failed to start Datadog profiler", "err", err)
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/app.go around lines 290-304, the Datadog profiler is started
unconditionally and panics on error and enables all profile types; change this
so profiler.Start errors are handled as non-fatal (log the error and continue
instead of panic), add an opt-in config flag (e.g., via appOpts) to gate
profiler startup, disable Block/Mutex/Goroutine by default (or make those
profile types configurable), and ensure profiler.Stop() is invoked exactly once
on clean shutdown (wire into the daemon shutdown hook or use defer in the
main/entrypoint).
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
Add DD profiler: