Skip to content

[o11y][nfc] Consolidate TailStreamWriterState | TailStreamWriter#5593

Merged
fhanau merged 3 commits intomainfrom
felix/112625-stw-refactor
Dec 10, 2025
Merged

[o11y][nfc] Consolidate TailStreamWriterState | TailStreamWriter#5593
fhanau merged 3 commits intomainfrom
felix/112625-stw-refactor

Conversation

@fhanau
Copy link
Copy Markdown
Contributor

@fhanau fhanau commented Nov 26, 2025

There was always a 1:1 mapping between TailStreamWriterState and TailStreamWriter, so the state struct being separate doesn't make sense.
This will make the STW implementation easier to maintain.

@fhanau fhanau requested a review from mar-cf November 26, 2025 19:11
@fhanau fhanau requested review from a team as code owners November 26, 2025 19:11
@fhanau
Copy link
Copy Markdown
Contributor Author

fhanau commented Dec 9, 2025

Reviewers: Using git diff -w is highly recommended, otherwise this change looks a lot larger than it actually is.

@fhanau fhanau force-pushed the felix/112625-stw-refactor branch from d1a9eba to 74b1bc0 Compare December 10, 2025 19:00
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Dec 10, 2025

CodSpeed Performance Report

Merging #5593 will not alter performance

Comparing felix/112625-stw-refactor (077d68e) with main (2aeb641)

Summary

✅ 57 untouched
⏩ 30 skipped1

Footnotes

  1. 30 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@fhanau fhanau force-pushed the felix/112625-stw-refactor branch from 74b1bc0 to 077d68e Compare December 10, 2025 20:55
@fhanau fhanau requested review from a team as code owners December 10, 2025 20:55
@fhanau
Copy link
Copy Markdown
Contributor Author

fhanau commented Dec 10, 2025

Added another commit here needed just for downstream lint checks to pass – with this change trace-stream.c++ is covered by the mutable globals check, so the static kj::StringPtr [] instances and corresponding js.obj() calls needed to be converted.

@jasnell
Copy link
Copy Markdown
Collaborator

jasnell commented Dec 10, 2025

Overall LGTM but will leave the actual sign off to Mar. I would say that I'm not sure the [nfc] label (non-functional change) is quite accurate given the extent of the updates here.

@fhanau
Copy link
Copy Markdown
Contributor Author

fhanau commented Dec 10, 2025

Overall LGTM but will leave the actual sign off to Mar. I would say that I'm not sure the [nfc] label (non-functional change) is quite accurate given the extent of the updates here.

It's not obvious based on the size of the diff, but this is mostly just moving code around. We are getting rid of the lambda by replacing it with a regular function and merge the functionality of TailStreamWriterState into TailStreamWriter, but the actual behavior should be the same as before.

@fhanau fhanau merged commit 36e75cd into main Dec 10, 2025
20 of 22 checks passed
@fhanau fhanau deleted the felix/112625-stw-refactor branch December 10, 2025 22:34
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.

3 participants