Resolve last direct apify/log calls

[l2ysho]

Followup to #3399

In these files are the last direct calls to apify/log, lets discuss if we need them.

1. packages/utils/src/internals/system-info/memory-info.ts
2. packages/utils/src/internals/system-info/cpu-info.ts
3. packages/utils/src/internals/sitemap.ts
4. packages/memory-storage/src/utils.ts
5. packages/memory-storage/src/background-handler/fs-utils.ts

[xrvnd]

Hi @l2ysho , I'd like to help with this if it's still open to outside contributors.
I took a quick look at the five files before commenting. A few observations:

• All 5 files import directly from @apify/log
• packages/core/src/log.ts is itself a re-export of @apify/log, so the files are functionally equivalent to using core's logger already
• Neither packages/utils nor packages/memory-storage have a local log module of their own
• Both packages appear to be dependencies of @crawlee/core — so importing from core would likely create a circular dependency

This makes me think the question isn't just "replace the import" but rather what the intended pattern is for lower-level packages in v4.

A couple of questions before I proceed:

1. Is this open for external contributions, or reserved for the core team given the v4 milestone?
2. Would the right approach be a small local log wrapper per package (similar to statistics.ts using defaultLog.child()), or is a shared @crawlee/log package planned for v4?

Happy to wait for guidance.

[l2ysho]

Hi @xrvnd, thanks for the thorough investigation before jumping in! I think these 5 are too low level to apply service locator in it, but lets wait for some words of wisdom also from @B4nan and @janbuchar

[janbuchar]

The only words of wisdom I can provide are:

1. maybe some of the utils don't actually need to log anything - we should remove log lines that don't provide any useful information
2. where logging is actually desired, using the serviceLocator is definitely the way to go so that we have uniform logging everywhere

[l2ysho]

probably 4. and 5. in memory storage can get serviceLocator, can't say if these logs in utils are the same, but feel wrong to me add serviceLocator to utils. (yeah because its utils in the first place). Also some latest findings

• apify/log should be removed from package.json in packages where serviceLocator is available
• probably we should write simple custom eslint rule to notifies devs to use serviceLocator instead of direct usage of apify/log (so we do not introduce it back)

[xrvnd]

Thanks for the guidance! To make sure I implement this correctly:

1. For memory-storage/src/utils.ts and fs-utils.ts — replace @apify/log with serviceLocator.getLogger()
2. For utils/src/internals/sitemap.ts, memory-info.ts, cpu-info.ts — review each log call and remove those that don't provide useful information. For the ones that do, also use serviceLocator ??
3. Should I also remove @apify/log from package.json in affected packages as part of this PR?
4. Must the ESLint rule be separate issue/PR or part of this one?

Happy to start once the scope is confirmed.

[janbuchar]

1. For `memory-storage/src/utils.ts` and `fs-utils.ts` — replace `@apify/log` with `serviceLocator.getLogger()`

Yes, that should be easy enough.

2. For `utils/src/internals/sitemap.ts`, `memory-info.ts`, `cpu-info.ts` — review each log call and remove those that don't provide useful information. For the ones that do, also use `serviceLocator` ??

Using serviceLocator from these will be tricky - in the current state of things, it will introduce a dependency cycle because @crawlee/core depends on @crawlee/utils. Introducing a new package just to sort this out sounds like an overkill - let's explore alternatives first.

3. Should I also remove `@apify/log` from `package.json` in affected packages as part of this PR?

I'd say yes. @l2ysho?

4. Must the ESLint rule be separate issue/PR or part of this one?

Not necessarily.

[l2ysho]

Thanks for the guidance! To make sure I implement this correctly:

1. For memory-storage/src/utils.ts and fs-utils.ts — replace @apify/log with serviceLocator.getLogger()
2. For utils/src/internals/sitemap.ts, memory-info.ts, cpu-info.ts — review each log call and remove those that don't provide useful information. For the ones that do, also use serviceLocator ??
3. Should I also remove @apify/log from package.json in affected packages as part of this PR?
4. Must the ESLint rule be separate issue/PR or part of this one?

Happy to start once the scope is confirmed.

3. this can be done here as a general cleanup
4. ESLint rule (if we decide to do it) is definitely out of scope of this issue.

not sure about rest, 1. seems ok to use serviceLocator and 2. no strong opinion for or against serviceLocator cc @janbuchar @B4nan

[l2ysho]

2. For `utils/src/internals/sitemap.ts`, `memory-info.ts`, `cpu-info.ts` — review each log call and remove those that don't provide useful information. For the ones that do, also use `serviceLocator` ??

did not see this (forgot to refresh). About the 2. there are some log.debug which can be omitted,log.warningOnce on the other hand make sense 🤔 For these few we can probably provide logger as an optional parameter, (or keep it this way, as usually it is some logging in catch block).

[janbuchar]

2. For `utils/src/internals/sitemap.ts`, `memory-info.ts`, `cpu-info.ts` — review each log call and remove those that don't provide useful information. For the ones that do, also use `serviceLocator` ??

did not see this (forgot to refresh). About the 2. there are some log.debug which can be omitted,log.warningOnce on the other hand make sense 🤔 For these few we can probably provide logger as an optional parameter, (or keep it this way, as usually it is some logging in catch block).

If it logs exceptions, maybe we could change the utils to simply throw and let the caller deal with the exception. Passing a logger that would only be used for exceptions sounds pretty bad.

[l2ysho]

Let run through these one by one and decide what to do

• memory-info.ts:92 — log.debug(...) — Lambda memory debug info
• memory-info.ts:126 — log.deprecated(...) — Warning: containerized env without cgroup memory support
• cpu-info.ts:65 — log.warningOnce(...) — Failed to get clock ticks, defaulting to 100
• cpu-info.ts:208 — log.deprecated(...) — Warning: containerized env without cgroup CPU support
• cpu-info.ts:237 — log.exception(...) — CPU snapshot failed, falling back to bare metal
• sitemap.ts:245 — log.debug(...) — Skipping sitemap due to max depth
• sitemap.ts:334 — log.warning(...) — Malformed sitemap content (with retry)
• sitemap.ts:342 — log.warning(...) — Malformed raw sitemap content
• sitemap.ts:454 — log.warning(...) — Failed to load sitemap, returning empty
• sitemap.ts:590 — log.debug(...) — Failed to check sitemap candidate URL
• sitemap.ts:567 — log.warning(...) — Failed to fetch robots.txt
• utils.ts:54 — defaultLog.child(...) — Creates shared memoryStorageLog (used by 3 files)
• fs-utils.ts:13 — log.child(...) — Creates backgroundHandlerLog for unknown message warnings

[janbuchar]

Thanks for the writeup @l2ysho. In my opinion:

1. memory-info.ts let's just remove those
2. cpu-info.ts same, but rethrow the error
   • but the check for containerized && missing cgroups API should probably be lifted up to SystemInfo or something like that
3. sitemap.ts either pass in a log instance via parameters, which makes for a bad API (if we make it required, it's annoying, if we don't we must either use some default logger or stay quiet)
   • or split sitemap/robots handling into a separate package that can then depend on @crawlee/core and use the service locator

[xrvnd]

Hi, just checking in on this. Happy to start implementing once the approach for sitemap.ts is decided — specifically whether to pass a logger as a parameter or split it into a separate package.

the directions seems to be clear for the memory-info.ts , cpu-info.ts , memory-storage/utils.ts and fs-utils.ts

Should I go ahead with these above while the sitemap.ts decision is pending or wait for the whole picture first??

I am happy to wait 👍