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

tests/e2e: revert logging all tx results #2653

Closed
ValarDragon opened this issue Sep 8, 2022 · 3 comments · Fixed by #3822
Closed

tests/e2e: revert logging all tx results #2653

ValarDragon opened this issue Sep 8, 2022 · 3 comments · Fixed by #3822
Assignees

Comments

@ValarDragon
Copy link
Member

Background

Not sure when it was added, but the e2e test suite is now logging all tx results. This should be reverted, as it massively crowds out the logs, and makes it hard to get relevant info at a glance. (Perhaps at most logging a tx hash by default, so you can quer)

If this is helpful for a particular debugging flow right now, we can add ways to improve that same debugging flows. (e.g. display results for last 3 txs)

@p0mvn
Copy link
Member

p0mvn commented Sep 8, 2022

I think this has significantly improved the debugging experience. If these are not present, it is close to impossible to tell what exactly happened in the failing container / why did a certain CLI command fail.

In the long term, we should instead pipe logs from each container to a file and preserve them as artifacts after running the CI job. This would remove the need for the extra logging. However, in the meantime, I think having more logs is better than having none

@ValarDragon
Copy link
Member Author

ValarDragon commented Sep 9, 2022

This is the log from the first TX I saw in the logs: https://pastebin.com/c94JZvwF

Its about 400 lines. Not sure which part of this is helpful for debugging in the normal case, beyond txhash, stdout, stderr. We can get the remainder of events & log from querying the txhash (osmosisd q tx {txhash}), but I don't see how this helps the standard case

WDYT about only logging this for a tx that fails, but not all succesful txs?

@p0mvn
Copy link
Member

p0mvn commented Sep 9, 2022

Its about 400 lines. Not sure which part of this is helpful for debugging in the normal case,

Yes, this is not useful at all for regular cases. However, the goal here is to log so that we can see the reasons for failure when it fails

WDYT about only logging this for a tx that fails, but not all succesful txs?

This is exactly the problem - it is hard to tell when a transaction is successful and when it is not in a general way

Off the top of my head, the problems are:

  • success is sometimes logged to stdout, other times in stderr
  • Some CLI commands log "error" to the output when they are successful so it is non-trivial to do some regex/pattern matching.

I'm open to investigating identifying the tx fails more. However, from my previous attempts, there are quite a few edge cases to work around. I can try looking into this again after v12

@p0mvn p0mvn self-assigned this Sep 9, 2022
Repository owner moved this from Needs Review 🔍 to Done ✅ in Osmosis Chain Development Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants