Skip to content

Consistent logging#154

Merged
peterzhu2118 merged 2 commits intoShopify:masterfrom
bquorning:consistent-logging
Feb 18, 2025
Merged

Consistent logging#154
peterzhu2118 merged 2 commits intoShopify:masterfrom
bquorning:consistent-logging

Conversation

@bquorning
Copy link
Contributor

Consistently log worker nr (where relevant) first, then generation, then pid. Use lowercase letters in most such messages.

Fixes #153

@bquorning
Copy link
Contributor Author

I pushed one more commit, adding the methods Worker#to_log and Service#to_log. I thought about using the method name #to_s which would make the interpolation easier – but at the same time, the output might be confusing in other use cases (e.g. when debugging).

I didn't yet squash the changes, so that it's easier to see the difference between the "inline" logging and the logging with #to_log. Of course the two commits should be squashed before merging.

The to_log method can be implemented in a few different ways (nested ifs, interpolating "#{" pid=#{pid}" if pid}", etc.), and I am not 100% happy with any of them. Please let me know if you have a cleaner solution.

@bquorning
Copy link
Contributor Author

Sorry about the force-push yesterday, but I had found four more locations to call to_log: https://github.com/Shopify/pitchfork/compare/7fde3c207694f5304cf7498246825bd5fe595cad..e787f9ede55e3d8d908fa389eb25f0d05af1de2e

@bquorning
Copy link
Contributor Author

Is there anything I can do to help this PR move forward?

@byroot
Copy link
Contributor

byroot commented Feb 17, 2025

Is there anything I can do to help this PR move forward?

Nah sorry, I'm away from work and so is Étienne, so we can't merge.

@peterzhu2118 I think you've been looking at Pitchfork lately? This PR looks good to me, it would be worth merging.

@peterzhu2118
Copy link
Contributor

@bquorning Thanks for the PR, I unblocked the tests and found that there's failures. Can you fix them?

@bquorning
Copy link
Contributor Author

Hmm, it seems I cannot run those tests locally:

$ bundle exec megatest test/integration/test_reforking.rb
No tests to run

but I’ll go though the CI output and fix the tests as best I can. And then rely on CI to see if I catch all failures 😅

@bquorning bquorning force-pushed the consistent-logging branch 3 times, most recently from 6da7982 to c6151ff Compare February 17, 2025 18:59
@bquorning
Copy link
Contributor Author

Hmm, it seems I cannot run those tests locally:

🤦🏼 because I’m testing on a Mac, where Pitchfork::REFORKING_AVAILABLE is false. I am testing it in Docker now.

Consistently log worker number (where relevant) first, then generation,
then pid. Use lowercase letters in most such messages.
Adding a helper method on Worker/Service to help making the log output
consistent. Depending on class, pid, etc., the method will return in one
of these 6 formats:

- mold gen=0
- mold gen=0 pid=57
- service gen=0
- service gen=0 pid=59
- worker=2 gen=0
- worker=2 gen=0 pid=58
@bquorning
Copy link
Contributor Author

There you go, tests are green now 🍏

Copy link
Contributor

@peterzhu2118 peterzhu2118 left a comment

Choose a reason for hiding this comment

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

Thank you!

@peterzhu2118 peterzhu2118 merged commit b70ee3c into Shopify:master Feb 18, 2025
10 checks passed
@bquorning bquorning deleted the consistent-logging branch May 15, 2025 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Small inconsistencies in log format

3 participants