Skip to content
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

preventing placeholder hosts from draining worker console logs #10345

Merged
merged 5 commits into from
Sep 11, 2024

Conversation

brettsam
Copy link
Member

@brettsam brettsam commented Jul 30, 2024

Issue describing the changes in this PR

Resolves #10222 and #9360

During specialization, there can be a race to process logs being generated by the placeholder worker. If a worker writes console output during startup (say, during a file load like the issue describes), the placeholder can dequeue these and log them, which effectively means they go nowhere.

This change prevents placeholder hosts from dequeuing these messages at all, so they are ready to be consumed by the user's host starting up.

Pull request checklist

IMPORTANT: Currently, changes must be backported to the in-proc branch to be included in Core Tools and non-Flex deployments.

  • Backporting to the in-proc branch is not required
    • Otherwise: Link to backporting PR
  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • My changes do not require diagnostic events changes
    • Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
  • I have added all required tests (Unit tests, E2E tests)

@brettsam brettsam requested a review from a team as a code owner July 30, 2024 22:38
@mattchenderson mattchenderson linked an issue Sep 6, 2024 that may be closed by this pull request
@kshyju
Copy link
Member

kshyju commented Sep 9, 2024

Do we need to add an entry to release notes for this change?

@brettsam brettsam force-pushed the brettsam/console_log branch from 984387f to 86e9851 Compare September 10, 2024 19:59
@surgupta-msft
Copy link
Contributor

surgupta-msft commented Sep 10, 2024

As mentioned in this issue - #9360, can this be a breaking change as users will start receiving logs which they were not getting before? Some logs will be errors/warnings and can have sensitive information as part of exception details (which can be a separate issue to fix but calling it out). Do we need to inform css about this as there could be more CRIs when users see additional errors/warnings in their appInsights?

@brettsam
Copy link
Member Author

@surgupta-msft -- good question. I wouldn't consider this a breaking change because the behavior isn't really changing today -- the problem is that during specialization, some logs disappeared.

But w/o specialization, it all worked (which is why it was tough to repro initially) -- so some customers would sometimes see these logs and other times not, depending on the specialization.

So i'd consider this a "fix" to make it consistently appear, even during specialization

@brettsam brettsam force-pushed the brettsam/console_log branch from ffa4c96 to 709e282 Compare September 11, 2024 15:07
@brettsam brettsam merged commit f58df65 into dev Sep 11, 2024
10 checks passed
@brettsam brettsam deleted the brettsam/console_log branch September 11, 2024 16:58
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.

Stdout/stderr logs during worker init not sent to app insights "Worker" logs do not show up in app insights
4 participants