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

Replace tracing #4017

Merged
merged 11 commits into from Jan 16, 2023
Merged

Replace tracing #4017

merged 11 commits into from Jan 16, 2023

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Jan 2, 2023 •
edited

Pull Request Description

Logging: Replace tracing with an efficient logging implementation, with 0-runtime cost for disabled log levels. (https://www.pivotaltracker.com/story/show/183755412)

Profiling: Support submitting profiler events to the User Timing Web API, so that measurements can be viewed directly in the browser. (https://www.pivotaltracker.com/story/show/184003550)

Important Notes

Logging interface:

  • The macros (warn!, etc.) now take standard format_args! arguments (the tracing implementations accepted a broader syntax).
  • Compile-time log levels can now be set through the CLI, like so:
    ./run ide start --log-level=trace --uncollapsed-log-level=info

Profiling:

  • The hotkey Ctrl+Alt+Shift+P submits all profiler events logged since the application was loaded to the Web API, so that they can then be viewed with the browser's developer tools. Note that standard tools are not able to represent async task lifetimes or metadata; this is a convenient interface to a subset of profiler data.
  • As an alternative interface, a runtime flag enables continuous measurement submission. In the browser it can be set through a URL parameter, like http://localhost:8080/?emit_user_timing_measurements=true. Note that this mode significantly impacts performance.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@kazcw kazcw self-assigned this Jan 2, 2023
@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 2, 2023
@kazcw kazcw marked this pull request as draft January 2, 2023 22:19
@kazcw
Copy link
Contributor Author

kazcw commented Jan 2, 2023 •
edited by unfurl-links bot

Back to draft: I'm adding a little more functionality tomorrow (https://www.pivotaltracker.com/story/show/184003550)

@kazcw kazcw marked this pull request as ready for review January 9, 2023 16:39
@wdanilo
Copy link
Member

wdanilo commented Jan 11, 2023

Is this ready for review? If so, please re-assign reviewers, so bot will ping them.

@kazcw kazcw marked this pull request as draft January 12, 2023 15:25
@kazcw kazcw marked this pull request as ready for review January 12, 2023 15:25
wdanilo
wdanilo approved these changes Jan 13, 2023
View reviewed changes
lib/rust/profiler/macros/src/low_level.rs
@@ -0,0 +1,197 @@
use crate::level;
Copy link
Member

Choose a reason for hiding this comment

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

  1. No module docs.
  2. Should this be named low_level? not log_level?

lib/rust/profiler/macros/src/level.rs Show resolved Hide resolved
lib/rust/profiler/macros/src/level.rs Show resolved Hide resolved
lib/rust/profiler/macros/src/profile_attribute.rs Show resolved Hide resolved
lib/rust/profiler/macros/src/wrap_async.rs Show resolved Hide resolved
@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Jan 16, 2023
@kazcw
Copy link
Contributor Author

kazcw commented Jan 16, 2023

@mwu-tow I need your review to make changes under /build. I've added a compile-time option to set the log-level.

mwu-tow
mwu-tow approved these changes Jan 16, 2023
View reviewed changes
build/cli/src/arg/wasm.rs
Comment on lines +61 to +62
#[derive(ArgEnum, Clone, Copy, Debug, PartialEq, Eq)]
pub enum LogLevel {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is good as it is, but the project/wasm.rs equivalent can derive ArgEnum as well and make this superfluous.
This is a tradeoff between separation (we keep CLI arguments representation separate from the environment variables for wasm) and duplication.

build/cli/src/arg/wasm.rs
Comment on lines +109 to +111
/// Compiles Enso with given log level. If not set, defaults to [`warn`].
#[clap(long, arg_enum, enso_env())]
pub log_level: Option<LogLevel>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than write what the default is in the documentation, I'd just remove Option, set default_value_t and let clap take care of presenting this,

build/cli/src/arg/wasm.rs
@@ -84,6 +106,14 @@ pub struct BuildInput {
#[clap(long, arg_enum, enso_env())]
pub profiling_level: Option<ProfilingLevel>,

/// Compiles Enso with given log level. If not set, defaults to [`warn`].
#[clap(long, arg_enum, enso_env())]
pub log_level: Option<LogLevel>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd consider prefixing this flag's name and the other one's with wasm_ prefix. Having just a --log-level among tons of projects is quite generic.

@mergify mergify bot merged commit 662992e into develop Jan 16, 2023
@mergify mergify bot deleted the wip/kw/replace-tracing branch January 16, 2023 20:31
@kazcw kazcw mentioned this pull request Jan 17, 2023
Merged
4 tasks
mergify bot pushed a commit that referenced this pull request Jan 19, 2023
@kazcw
Rename option to --wasm-log-level; small refactoring (#4060)
f39070a 
Rename the CLI option to set the WASM log level; applied some suggested simplifications, to `LogLevel` and to the type it was based on.

# Important Notes
This addresses review of #4017.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mwu-tow mwu-tow mwu-tow approved these changes

@wdanilo wdanilo wdanilo approved these changes

@MichaelMauderer MichaelMauderer Awaiting requested review from MichaelMauderer

@farmaazon farmaazon Awaiting requested review from farmaazon farmaazon is a code owner

Assignees

@kazcw kazcw

Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kazcw @wdanilo @mwu-tow