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.

Sign up for GitHub

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

Jump to bottom

Panic because WasmVM cache metrics collector cannot be registered properly #1574

Closed
dadamu opened this issue Aug 28, 2023 · 0 comments · Fixed by #1575
Closed

Panic because WasmVM cache metrics collector cannot be registered properly #1574

dadamu opened this issue Aug 28, 2023 · 0 comments · Fixed by #1575

Comments

@dadamu
Copy link
Contributor

dadamu commented Aug 28, 2023

Description

WasmVM metrics now is not registered properly and it occurs panics when querying the /metrics endpoint. The panic message is as follows:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x1f5b9dd]

goroutine 148 [running]:
github.com/CosmWasm/wasmd/x/wasm/keeper.(*WasmVMMetricsCollector).Collect(0xc0009fa660, 0xc000d25f60?)
	github.com/CosmWasm/wasmd/x/wasm/keeper/metrics.go:56 +0x3d
github.com/prometheus/client_golang/prometheus.(*Registry).Gather.func1()
	github.com/prometheus/client_golang@v1.16.0/prometheus/registry.go:455 +0x10d
created by github.com/prometheus/client_golang/prometheus.(*Registry).Gather
	github.com/prometheus/client_golang@v1.16.0/prometheus/registry.go:547 +0xbac

How to repoduce

  1. setup wasmd testnet
  2. enable instrumentation.prometheus inside ~/.wasmd/config/config.toml
  3. enable telemetry.enabled inside ~/wasmd/config/app.toml
  4. start wasmd localnet by wasmd start
  5. query /metrics by curl localhost:26660/metrics
  6. node panics

How to fix

Fix WithVMCacheMetrics to use postOptsFn instead of optsFn to make sure wasmVM has already been setup.

func WithVMCacheMetrics(r prometheus.Registerer) Option {
-	  return optsFn(func(k *Keeper) {
+	  return postOptsFn(func(k *Keeper) {
		NewWasmVMMetricsCollector(k.wasmVM).Register(r)
	})
}

https://github.com/CosmWasm/wasmd/blob/main/x/wasm/keeper/options.go#L129
@dadamu dadamu changed the title panic because wasmVM metrics collector cannot be registered properly Panic because WasmVM cache metrics collector cannot be registered properly Aug 28, 2023
@dadamu dadamu mentioned this issue Aug 28, 2023
Merged
This was referenced Aug 28, 2023
Merged
Closed
@wojtek-coreum wojtek-coreum mentioned this issue Sep 6, 2023
Merged
6 tasks
wojtek-coreum added a commit to CoreumFoundation/coreum that referenced this issue Sep 7, 2023
@wojtek-coreum
Turn off buggy WASM metrics (#637)
00d4a8d 
# Description

Due to buggy initialization of metrics collector, WASM causes panics when metrics are collected.
Bug is described here: CosmWasm/wasmd#1574
and fixed here: CosmWasm/wasmd#1575

Once v0.42 is released we may uncomment this code. Task has been created for this.

# Reviewers checklist:
- [ ] Try to write more meaningful comments with clear actions to be taken.
- [ ] Nit-picking should be unblocking. Focus on core issues.

# Authors checklist
- [x] Provide a concise and meaningful description
- [x] Review the code yourself first, before making the PR.
- [x] Annotate your PR in places that require explanation.
- [x] Think and try to split the PR to smaller PR if it is big.
@dadamu dadamu mentioned this issue Sep 11, 2023
Merged
19 tasks
RiccardoM added a commit to desmos-labs/desmos that referenced this issue Sep 12, 2023
@dadamu @RiccardoM
fix: disable buggy wasm metrics (#1235)
6db3204 
## Description

This PR disables the buggy wasm metrics in order to avoid node running
failure.

Relates to CosmWasm/wasmd#1574


<!-- Add a description of the changes that this PR introduces and the
files that
are the most critical to review. -->

---

### Author Checklist

*All items are required. Please add a note to the item if the item is
not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type
prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json)
in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR
Targeting](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building
modules](https://docs.cosmos.network/v0.44/building-modules/intro.html)
- [ ] included the necessary unit and integration
[tests](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go
code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable
and please add
your handle next to the items reviewed if you only reviewed selected
items.*

I have...

- [ ] confirmed the correct [type
prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json)
in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

---------

Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
RiccardoM pushed a commit to desmos-labs/desmos that referenced this issue Sep 12, 2023
@dadamu @RiccardoM
fix: disable buggy wasm metrics (#1235)
fc6cdaf 
## Description

This PR disables the buggy wasm metrics in order to avoid node running
failure.

Relates to CosmWasm/wasmd#1574

<!-- Add a description of the changes that this PR introduces and the
files that
are the most critical to review. -->

---

### Author Checklist

*All items are required. Please add a note to the item if the item is
not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type
prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json)
in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR
Targeting](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building
modules](https://docs.cosmos.network/v0.44/building-modules/intro.html)
- [ ] included the necessary unit and integration
[tests](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go
code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable
and please add
your handle next to the items reviewed if you only reviewed selected
items.*

I have...

- [ ] confirmed the correct [type
prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json)
in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

---------

Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
(cherry picked from commit 6db3204)
braverdever added a commit to braverdever/Coreum that referenced this issue Oct 24, 2023
@braverdever
Turn off buggy WASM metrics (#637)
add750a 
# Description

Due to buggy initialization of metrics collector, WASM causes panics when metrics are collected.
Bug is described here: CosmWasm/wasmd#1574
and fixed here: CosmWasm/wasmd#1575

Once v0.42 is released we may uncomment this code. Task has been created for this.

# Reviewers checklist:
- [ ] Try to write more meaningful comments with clear actions to be taken.
- [ ] Nit-picking should be unblocking. Focus on core issues.

# Authors checklist
- [x] Provide a concise and meaningful description
- [x] Review the code yourself first, before making the PR.
- [x] Annotate your PR in places that require explanation.
- [x] Think and try to split the PR to smaller PR if it is big.
braverdever pushed a commit to swordEdge/Coreum that referenced this issue Oct 24, 2023
@swordEdge
Turn off buggy WASM metrics (#637)
bce6bbb 
# Description

Due to buggy initialization of metrics collector, WASM causes panics when metrics are collected.
Bug is described here: CosmWasm/wasmd#1574
and fixed here: CosmWasm/wasmd#1575

Once v0.42 is released we may uncomment this code. Task has been created for this.

# Reviewers checklist:
- [ ] Try to write more meaningful comments with clear actions to be taken.
- [ ] Nit-picking should be unblocking. Focus on core issues.

# Authors checklist
- [x] Provide a concise and meaningful description
- [x] Review the code yourself first, before making the PR.
- [x] Annotate your PR in places that require explanation.
- [x] Think and try to split the PR to smaller PR if it is big.
Superdev1010 added a commit to Superdev1010/Coreum that referenced this issue Mar 4, 2024
@Superdev1010
Turn off buggy WASM metrics (#637)
0d7025a 
# Description

Due to buggy initialization of metrics collector, WASM causes panics when metrics are collected.
Bug is described here: CosmWasm/wasmd#1574
and fixed here: CosmWasm/wasmd#1575

Once v0.42 is released we may uncomment this code. Task has been created for this.

# Reviewers checklist:
- [ ] Try to write more meaningful comments with clear actions to be taken.
- [ ] Nit-picking should be unblocking. Focus on core issues.

# Authors checklist
- [x] Provide a concise and meaningful description
- [x] Review the code yourself first, before making the PR.
- [x] Annotate your PR in places that require explanation.
- [x] Think and try to split the PR to smaller PR if it is big.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: make sure wasmvm cache metrics collector registration after VM setup
1 participant
@dadamu