Implement a custom event formatter for javascript logging

[richvdh]

Somehow fmt::pretty manages to be both overcomplicated and not flexible enough.

The driver here is that I want to put the event "fields" on a separate line to the message.

[codecov]

Codecov Report

Attention: 39 lines in your changes are missing coverage. Please review.

Comparison is base (5786b1a) 82.33% compared to head (6591a6e) 82.18%.
Report is 40 commits behind head on main.

Files	Patch %	Lines
crates/matrix-sdk-common/src/js_tracing.rs	0.00%	39 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2856      +/-   ##
==========================================
- Coverage   82.33%   82.18%   -0.16%     
==========================================
  Files         211      211              
  Lines       21620    21659      +39     
==========================================
- Hits        17801    17800       -1     
- Misses       3819     3859      +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[richvdh]

I think the coverage metrics must be wrong here. There is definitely a test that runs this code and checks the result. I think it is run here although the test name seems to have sprouted an _outer suffix from somewhere.

[jplatte]

We don't (and probably can't) collect coverage on WASM-only code. You can change the cfg(target_arch = "wasm32") attribute on the js_tracing module to cfg(all(target_arch = "wasm32", not(tarpaulin_include))) to exclude it from coverage reporting.

[richvdh]

You can change the cfg(target_arch = "wasm32") attribute on the js_tracing module to cfg(all(target_arch = "wasm32", not(tarpaulin_include))) to exclude it from coverage reporting.

Is that the right thing to do? If we're unable to measure code coverage on a portion of our code, it's probably better that our metrics reflect that, even if it means overriding the coverage check on PRs that touch that code?

[jplatte]

I don't think I have the authority to answer that question 😄

I'm fine merging with or without that.

[richvdh]

In this case, it should be possible to test JsEventFormatter outside wasm, since it doesn't rely on the js console. But that would mean bringing tracing-subscriber as a (dev?) dependency, and building parts of the js_tracing module outside wasm, and suddenly it's a whole bunch of work just to satisfy a coverage metric.

[poljar]

Is that the right thing to do? If we're unable to measure code coverage on a portion of our code, it's probably better that our metrics reflect that, even if it means overriding the coverage check on PRs that touch that code?

The other argument can be made as well, if we know that the code is tested, why do we want code coverage to tell us it isn't?

I think it's fine to disable coverage collection in this case, because:

1. The code is tested.
2. The bot will pester us needlessly that we should add a test for it when we don't need to.
3. People looking at the coverage reports will see a file with 0% code coverage and will try to write test for it, when in fact there are tests.

Out of those, no 3 sounds the worst to me. Disabling the gathering with a comment explaining that we have tests sounds sensible to avoid this situation. What exactly do we gain from not disabling it?

On the other hand, I have trouble understanding why this piece of code is even in the SDK repo.

[richvdh]

Is that the right thing to do? If we're unable to measure code coverage on a portion of our code, it's probably better that our metrics reflect that, even if it means overriding the coverage check on PRs that touch that code?

The other argument can be made as well, if we know that the code is tested, why do we want code coverage to tell us it isn't?

I think it's fine to disable coverage collection in this case, because:

1. The code is tested.
2. The bot will pester us needlessly that we should add a test for it when we don't need to.
3. People looking at the coverage reports will see a file with 0% code coverage and will try to write test for it, when in fact there are tests.

Out of those, no 3 sounds the worst to me. Disabling the gathering with a comment explaining that we have tests sounds sensible to avoid this situation. What exactly do we gain from not disabling it?

Those are fair arguments, particularly because in this case it is, in fact, tested. The counterpoint is if we start to accumulate (even more) cruft in this module which is not tested, and the metric won't pick it up. But let's agree not to do that.

On the other hand, I have trouble understanding why this piece of code is even in the SDK repo.

Fair question. The reason that MakeJsLogWriter is in this repo is basically so that we can do logging from within tests of other parts of the SDK that have to be done in the wasm environment (eg the indexeddb store). And I dumped JsEventFormatter here because they went together in my head, but that's not actually right and I could certainly move it to the wasm bindings repo if you prefer.

[poljar]

And I dumped JsEventFormatter here because they went together in my head, but that's not actually right and I could certainly move it to the wasm bindings repo if you prefer.

It sounds like you'd like to put this into a separate crate, the IndexedDB crate and the WASM bindings can then use this new crate. Something like https://github.com/jquesada2016/tracing_subscriber_wasm or https://github.com/old-storyai/tracing-wasm might also be interesting for the IndexedDB/tests case.

In any case, I'm fine with disabling the gathering of the coverage and moving forward with this PR.

[richvdh]

Something like https://github.com/jquesada2016/tracing_subscriber_wasm or https://github.com/old-storyai/tracing-wasm might also be interesting for the IndexedDB/tests case.

Hrm yes. I hadn't found those before. One thing I wanted to achieve with MakeJsLogWriter was the ability to pass in a "Logger" object other than the console, and tracing_subscriber_wasm lacks that ability. Still, it would make a lot of sense to use tracing_subscriber_wasm in the tests and then we could put this js_logging module into the WASM bindings of the rust-sdk where it probably belongs. (Or, as you say, in a completely separate crate). I'll bear this in mind for the future.

I also like tracing_wasm's idea of wiring up spans to the JS Performance API. It looks like it might be really helpful in tracking down performance problems. Something to explore in future.

In any case, I'm fine with disabling the gathering of the coverage and moving forward with this PR.

Ok thanks; that is what I have done for now.

[jplatte]

Suggested change

	pub struct JsEventFormatter {}
	#[non_exhaustive]
	pub struct JsEventFormatter {}

If we want to enforce that ::new() is the way to create an instance.