-
Notifications
You must be signed in to change notification settings - Fork 27
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
Turn off buggy WASM metrics #637
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil, @vertex451, and @ysv)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @vertex451, @wojtek-coreum, and @ysv)
app/app.go
line 567 at r2 (raw file):
// FIXME (wasmd-1575): This is commented out temporarily because it causes panics in telemetry server due to buggy // initialization of wasm vm in version v0.41 of wasmd.
But if it's fixed probably it's better to update the dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @vertex451, @wojtek-coreum, and @ysv)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @vertex451, and @ysv)
app/app.go
line 567 at r2 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
But if it's fixed probably it's better to update the dependency?
To the latest commit in master? But I'm not sure they follow the same very useful policy that every commit in master should work. That's why I prefer to wait until new official release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @vertex451 and @ysv)
app/app.go
line 567 at r2 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
To the latest commit in master? But I'm not sure they follow the same very useful policy that every commit in master should work. That's why I prefer to wait until new official release.
Ok, agree. Do we have a corresponding task to enable it back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @vertex451)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @vertex451)
app/app.go
line 567 at r2 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
Ok, agree. Do we have a corresponding task to enable it back?
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @vertex451)
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:
Authors checklist
This change is