Skip to content

chore: nuking debug-only logger and various unused functionality in foundation#13187

Merged
benesjan merged 6 commits intomasterfrom
03-31-chore_nuking_debug-only_logger
Apr 3, 2025
Merged

chore: nuking debug-only logger and various unused functionality in foundation#13187
benesjan merged 6 commits intomasterfrom
03-31-chore_nuking_debug-only_logger

Conversation

@benesjan
Copy link
Contributor

@benesjan benesjan commented Mar 31, 2025

Fixes #11061

Behavior of DebugOnlyLogger was inconsistent with the standard logger and the logger was not really needed anymore as the debug function of standard logger can be used to achieve the same result.

Some of the code that used the debug only logger (foundation/worker) was no longer used so I pruned that as well. Other than that Adam pointed out that only bb.js cares about worker_listener.ts so I pruned that (and related) functionality as well (foundation/src/transport/browser dir).

I've also nuked foundation/src/wasm as it was also apparently unused.

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@benesjan benesjan force-pushed the 03-31-chore_nuking_debug-only_logger branch from 5d47299 to 3c0d5a1 Compare March 31, 2025 19:22
interface Printable {
toString(): string;
}
export function applyStringFormatting(formatStr: string, args: Printable[]): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this function to noir_debug_log_util.js

* of recent logs, or clear stored logs based on a given count. This can be useful for debugging
* purposes, monitoring application activities, and maintaining log history.
*/
export class LogHistory {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was unused and imported debug logger related functionality so I nuked it.

I have very little context here so not sure if we might ever need it again.

@benesjan benesjan added the ci-full Run all master checks. label Mar 31, 2025
@benesjan benesjan marked this pull request as ready for review March 31, 2025 19:38
@benesjan benesjan requested a review from spalladino March 31, 2025 21:37
trace = (msg: string, data?: unknown) =>
this.transportServer.broadcast({ fn: 'emit', args: ['log', 'trace', msg, data] });

readonly level = 'trace' as const;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if we want to respect the log levels here given that the original logger in the context where this was used was a debug logger.

Copy link
Contributor

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this @benesjan! I checked and start_web_module and start_node_module are not used anywhere. I propose we delete them, which allows us to also delete the worker_logger, which also allows us to delete the wasm_module#addLogger function. WDYT?

@benesjan benesjan force-pushed the 03-31-chore_nuking_debug-only_logger branch 2 times, most recently from 7e42f48 to 26c3b85 Compare April 2, 2025 17:04
@benesjan benesjan changed the title chore: nuking debug-only logger chore: nuking debug-only logger, foundation/transport/browser, foundation/worker Apr 2, 2025
@benesjan benesjan requested review from ludamad and spalladino April 2, 2025 18:25
@@ -1,4 +1,4 @@
import { createDebugOnlyLogger } from '../log/index.js';
import { createLogger } from '../log/index.js';

Copy link
Collaborator

Choose a reason for hiding this comment

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

isnt this entirely unused?

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is now defined in the C++

Copy link
Collaborator

Choose a reason for hiding this comment

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

for that matter, the whole wasm folder in foundation is not used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Nuked that whole dir as well in adf51d5

@benesjan benesjan force-pushed the 03-31-chore_nuking_debug-only_logger branch from 26c3b85 to adf51d5 Compare April 2, 2025 19:20
@benesjan benesjan changed the title chore: nuking debug-only logger, foundation/transport/browser, foundation/worker chore: nuking debug-only logger and various unused functionality in foundation Apr 2, 2025
@benesjan benesjan requested review from ludamad and spalladino and removed request for ludamad and spalladino April 2, 2025 19:55
Copy link
Contributor

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

Absolutely beautiful

@benesjan benesjan added this pull request to the merge queue Apr 3, 2025
Merged via the queue into master with commit 2d38e60 Apr 3, 2025
11 checks passed
@benesjan benesjan deleted the 03-31-chore_nuking_debug-only_logger branch April 3, 2025 22:33
github-merge-queue bot pushed a commit that referenced this pull request Apr 4, 2025
🤖 I have created a new Aztec Packages release
---


##
[0.84.0](v0.83.1...v0.84.0)
(2025-04-04)


### ⚠ BREAKING CHANGES

* `UnsconstrainedContext` --> `UtilityContext`
([#13246](#13246))
* `#[utility]` function
([#13243](#13243))
* Validate public setup fns and gas in p2p
([#13154](#13154))

### Features

* `#[utility]` function
([#13243](#13243))
([945ffa2](945ffa2))
* **avm:** tx hint init
([#13218](#13218))
([60a1a92](60a1a92))
* Remove 4 byte metadata from bb-produced proof
([#13231](#13231))
([0dcc915](0dcc915))
* To enable better ci dashboard.
([#13272](#13272))
([61c6375](61c6375))


### Bug Fixes

* **avm:** fix lookup builder and FF hashing
([#13263](#13263))
([2633856](2633856))
* ci3-external concurrency bug, reduce grind set
([2c5e830](2c5e830)),
closes
[#13285](#13285)
* ci3-external.yml
([#13291](#13291))
([6ad68ed](6ad68ed))
* Validate public setup fns and gas in p2p
([#13154](#13154))
([1ef4add](1ef4add)),
closes
[#10958](#10958)


### Miscellaneous

* `UnsconstrainedContext` --> `UtilityContext`
([#13246](#13246))
([69df86f](69df86f))
* add some PrivateSet tests
([#13270](#13270))
([bd9e690](bd9e690))
* bump full prover test to 32 cores. hoping to boost speed.
([#13293](#13293))
([c8e95dd](c8e95dd))
* deflake p2p reqresp test
([#13271](#13271))
([b9164fa](b9164fa))
* don't dump on fail. click the link instead.
([#13292](#13292))
([ba0fb4d](ba0fb4d))
* flake
([#13277](#13277))
([62c32eb](62c32eb))
* make rahul happy with migration notes
([#13255](#13255))
([3dd75a6](3dd75a6))
* minor simulator utils cleanup
([#13250](#13250))
([8a622c9](8a622c9))
* move a couple of `SharedMutableValues` functions outside of impl
([#13283](#13283))
([df9a40c](df9a40c))
* nuking debug-only logger and various unused functionality in
`foundation`
([#13187](#13187))
([2d38e60](2d38e60))
* prevent eth devnet config contention in ci
([#13260](#13260))
([1581836](1581836))
* renaming unconstrained function as utility in TS
([#13249](#13249))
([34d03bb](34d03bb))
* replace relative paths to noir-protocol-circuits
([b5b99f8](b5b99f8))
* Speed up note hashes test
([#13282](#13282))
([ad23358](ad23358))
* update gov and proposer configs
([#13281](#13281))
([e1a5be3](e1a5be3))
* update slashing test port
([#13274](#13274))
([9a1ddc5](9a1ddc5))
* Want to fail fast on test runs and the wider ci run.
([#13258](#13258))
([f0553b8](f0553b8))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-full Run all master checks.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: Remove debug usage on wasm modules

3 participants