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

Guard performance.* with report_logs_in_timings #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jenanwise
Copy link

I'm not sure if you intend for this library to be used outside of browsers. See #3 for that question. If so, currently, tracing-wasm doesn't work in environments where the global performance object is not available. E.g.:

  • nodejs exports it as perf_hooks.performance, not as a global peformance object.

  • cloudflare workers have a limited subset of global objects which does not include performance at all.

WASMLayerConfig allows opt-out of the performance timing APIs with the report_logs_in_timings option, but it was not previously being checked before some usage of performance.mark and performance.measure. This patch guards those calls against the config option.

The global `performance` object is not available in some javascript
enviroments. E.g.:

- nodejs exports it as `perf_hooks.performance`, not as a global
`peformance` object.

- cloudflare workers have a limited subset of global objects which does
not include `performance` at all.

`WASMLayerConfig` allows opt-out of the performance timing APIs with the
`report_logs_in_timings` option, but it was not previously being checked
before some usage of `performance.mark` and `performance.measure`. This
patch guards those calls against the config option.
@colelawrence
Copy link
Member

Hi @jenanwise, for this use-case, I think there is a way to expand the configuration options to affect which binding for performance marks is used. So, this feels more like an additional "wasm_context" configuration value, that could be Browser, NodeJS, or ConsoleOnly.

WDYT?

@jenanwise
Copy link
Author

@colelawrence I'm not sure I follow. This PR just uses the already-in-place boolean report_log_in_timings consistently, unless I'm misunderstanding its purpose. Why add an additional configuration option?

@jenanwise
Copy link
Author

To expand: as an additional step, you could either dynamically fetch the right performance object (I would recommend this -- e.g. see how js_sys::global() works) or allow explicit configuration, but that seems independent of allowing completely opting out of all timings APIs, given the already-existing configuration options.

@jenanwise jenanwise mentioned this pull request Aug 14, 2020
@colelawrence
Copy link
Member

Sorry for coming back to this late so late, but I think the goal was to make it so you could disable little ticks in the performance spans API if you didn't want the logging duplication (between performance and console.log).

I think we should rename the existing config name to should_report_events_in_timings (default true) and then add a should_report_spans_in_timings (default true) option for guarding enter and exit.

We could also make should_report_events_in_console (default true) an option as well.

WDYT?

@jenanwise
Copy link
Author

@colelawrence That sounds reasonable to me. I haven't thought about wasm in almost a year though, so I will defer to you at this point 😄

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

Successfully merging this pull request may close these issues.

2 participants