Skip to content

fix: respect child logger factory in fastify options#6349

Merged
Eomm merged 2 commits intofastify:mainfrom
cysp:fastify-options-child-logger-factory
Oct 7, 2025
Merged

fix: respect child logger factory in fastify options#6349
Eomm merged 2 commits intofastify:mainfrom
cysp:fastify-options-child-logger-factory

Conversation

@cysp
Copy link
Contributor

@cysp cysp commented Oct 4, 2025

Summary

Since 2695ada, fastify's initialisation options allow provision of a childLoggerFactory function as an alternative to the .setChildLoggerFactory() method for setting a child logger factory on an instance. However, this initialisation option is ignored, with the child logger factory always taking the value of the defaultChildLoggerFactory.

This change will allow provision of a child logger factory through fastify's initialiser.
I have not introduced an additional test to cover this behaviour, but have instead corrected the implementation of an existing test that was intended to cover it.

Checklist

  • run npm run test && npm run benchmark --if-present
  • tests and/or benchmarks are included
  • documentation is changed or added
    • not sure if any documentation should be added here, please advise if so :-)
  • commit message and code follows the Developer's Certification of Origin
    and the Code of conduct
Test output summary
ℹ tests 2139
ℹ suites 15
ℹ pass 2136
ℹ fail 0
ℹ cancelled 0
ℹ skipped 3
ℹ todo 0
ℹ duration_ms 10508.554167
Benchmark output
> fastify@5.6.1 benchmark
> concurrently -k -s first "node ./examples/benchmark/simple.js" "autocannon -c 100 -d 30 -p 10 localhost:3000/"

[1] Running 30s test @ http://localhost:3000/
[1] 100 connections with 10 pipelining factor
[1]
[1]
[1] ┌─────────┬──────┬───────┬───────┬───────┬─────────┬─────────┬────────┐
[1] │ Stat    │ 2.5% │ 50%   │ 97.5% │ 99%   │ Avg     │ Stdev   │ Max    │
[1] ├─────────┼──────┼───────┼───────┼───────┼─────────┼─────────┼────────┤
[1] │ Latency │ 6 ms │ 12 ms │ 15 ms │ 15 ms │ 10.3 ms │ 4.85 ms │ 388 ms │
[1] └─────────┴──────┴───────┴───────┴───────┴─────────┴─────────┴────────┘
[1] ┌───────────┬─────────┬─────────┬─────────┬─────────┬───────────┬──────────┬─────────┐
[1] │ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg       │ Stdev    │ Min     │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼───────────┼──────────┼─────────┤
[1] │ Req/Sec   │ 88,191  │ 88,191  │ 92,479  │ 97,151  │ 92,663.47 │ 1,768.19 │ 88,172  │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼───────────┼──────────┼─────────┤
[1] │ Bytes/Sec │ 16.6 MB │ 16.6 MB │ 17.4 MB │ 18.3 MB │ 17.4 MB   │ 332 kB   │ 16.6 MB │
[1] └───────────┴─────────┴─────────┴─────────┴─────────┴───────────┴──────────┴─────────┘
[1]
[1] Req/Bytes counts sampled once per second.
[1] # of samples: 30
[1]
[1] 2781k requests in 30.02s, 523 MB read
[1] autocannon -c 100 -d 30 -p 10 localhost:3000/ exited with code 0

@gurgunday gurgunday requested a review from a team October 4, 2025 08:46
@cysp
Copy link
Contributor Author

cysp commented Oct 6, 2025

ah, I've seen this test flake locally but it appears to be unrelated to this change

test at test/issue-4959.test.js:46:1
✖ should handle a socket error (21.455625ms)
  'plan expected 4 assertions but received 1'

(it has also failed in some other prs: https://github.com/fastify/fastify/actions/runs/18059271339/job/51393309540?pr=6229#step:5:2360, https://github.com/fastify/fastify/actions/runs/17595152215/job/49985744216?pr=6311#step:5:2342)

@Eomm Eomm added bugfix Issue or PR that should land as semver patch typescript TypeScript related labels Oct 7, 2025
@Eomm Eomm merged commit e1f58ab into fastify:main Oct 7, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@metcoder95 metcoder95 metcoder95 approved these changes

@Eomm Eomm Eomm approved these changes

@gurgunday gurgunday gurgunday approved these changes

Assignees

No one assigned

Labels

bugfix Issue or PR that should land as semver patch typescript TypeScript related

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cysp @metcoder95 @Eomm @gurgunday