Skip to content

Fix missing commas in logs #7361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Conversation

ackintosh
Copy link
Member

@ackintosh ackintosh commented Apr 27, 2025

Issue Addressed

In beacon.log, commas are missing in case where multiple spans are present, for example:

Apr 05 07:03:07.646 DEBUG Sync's view on execution engine state updated  past_state: Online, new_state: Offline, service: "sync"service: "network_context"

A comma should be present between service: "sync" and service: "network_context" as shown in the example.

Proposed Changes

This PR:

  • makes build_log_text() testable and adds unit tests for it
  • fixes the missing commas issue

@ackintosh ackintosh marked this pull request as ready for review April 27, 2025 22:49
@ackintosh ackintosh added the ready-for-review The code is ready for review label Apr 27, 2025
Copy link
Member

@macladson macladson left a comment

Choose a reason for hiding this comment

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

This has some of the same ideas as #7239, I'm wondering if you've had a chance to look at that PR? It would be good if we could make them compatible with each other.

@macladson
Copy link
Member

I've written up a version of this PR on top of #7239
There's a few changes (commas are handled slightly differently and the span order for multiple spans is reversed).
I wanted to make sure you still get the credit for this fix so let me know what you think.

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 15, 2025
@mergify mergify bot added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jun 14, 2025
@mergify mergify bot closed this Jun 14, 2025
Copy link

mergify bot commented Jun 14, 2025

Hi @ackintosh, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

@mergify mergify bot added the stale Stale PRs that have been inactive and is now outdated label Jun 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review The code is ready for review stale Stale PRs that have been inactive and is now outdated UX-and-logs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants