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

Improvements to the tracing and logging in wasmer #3644

Merged
merged 7 commits into from
Mar 7, 2023
Merged

Conversation

john-sharratt
Copy link
Contributor

  • Now using the fmt subscriber for improved formatting
  • Using spans on all syscalls
  • Added recorded fields on the spans instead trace and debug macros
  • Adding timing on all syscall

Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

This looks great. I wished we did this earlier. Good work!

Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

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

Very nice @john-sharratt !

One concern: since often the syscall args are in the spans now this has the potential to produce extremely noisy logs, we might want to move some of those back to just logging them in a first log message inside the syscall.

But we can just change that as we go along.

lib/cli/src/logging.rs Outdated Show resolved Hide resolved
@john-sharratt
Copy link
Contributor Author

Very nice @john-sharratt !

One concern: since often the syscall args are in the spans now this has the potential to produce extremely noisy logs, we might want to move some of those back to just logging them in a first log message inside the syscall.

But we can just change that as we go along.

the spans all have skip-all turned on by default - which means the number of fields have to be explicitly added and won't grow, but it might be an idea to add a feature flag for super noisy syscalls.

to be honest i think we should do the opposite though as I commended out the thread_local_get syscall debug and trace messages because they were so noisy... but then later (as in a few days ago) when I put the spans on i noticed there were millions of them now because of tokio going nuts with locals.

i think we should leave it noisy when in tracing mode, make it "useful" in debug level.... and optimize the code so that less syscalls are made. i mean the syscalls will be killing performance anyway

@theduke
Copy link
Contributor

theduke commented Mar 6, 2023

@john-sharratt small misunderstanding:

All syscalls should be instrumented, totally.

But we can chose between either including the fields in instrument(), which means they will be re-printed on each log message inside the created span, or just logging the values once with a manual trace!() which means they won't be in the span.

And I think we will mostly want to do the latter, except for very small stuff like maybe the fd.

john-sharratt and others added 4 commits March 7, 2023 09:43
- Now using the fmt subscriber for improved formatting
- Using spans on all syscalls
- Added recorded fields on the spans instead trace and debug macros
- Adding timing on all syscall
No point in keeping two implementations around
@theduke theduke enabled auto-merge (rebase) March 7, 2023 10:38
@theduke theduke merged commit 202d9f1 into master Mar 7, 2023
@theduke theduke deleted the better-tracing branch March 7, 2023 11:15
@john-sharratt
Copy link
Contributor Author

@john-sharratt small misunderstanding:

All syscalls should be instrumented, totally.

But we can chose between either including the fields in instrument(), which means they will be re-printed on each log message inside the created span, or just logging the values once with a manual trace!() which means they won't be in the span.

And I think we will mostly want to do the latter, except for very small stuff like maybe the fd.

Ah I understand - yeah that makes sense - we'll work towards that approach going forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants