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

Set slogger on rungroup after logshipping is set up, so that rungroup logs will be shipped #1869

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

RebeccaMahany
Copy link
Contributor

Despite the changes in #1822, I wasn't seeing rungroup shutdown logs being shipped. When looking through the shipped logs, I noticed that the only rungroup logs available were either from very old versions of launcher, or from the desktop process.

This PR updates when we set the slogger on the rungroup so that we set it after the shipping handler has been added.

directionless
directionless previously approved these changes Sep 17, 2024
Copy link
Contributor

@directionless directionless left a comment

Choose a reason for hiding this comment

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

Hrm. Not sure I love the changed signature. It feels error prone.

We could maybe move NewRunGroup to after the logshipper is setup? Though it would mean moving runGroup.Add till later too

Copy link
Contributor

@zackattack01 zackattack01 left a comment

Choose a reason for hiding this comment

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

great find! this looks good to me I think you just have to fix up the watchdog_service_windows NewRunGroup call - those are only written to sqlite so I don't think order/timing should matter at all

@RebeccaMahany
Copy link
Contributor Author

RebeccaMahany commented Sep 17, 2024

@directionless

Hrm. Not sure I love the changed signature. It feels error prone.

We could maybe move NewRunGroup to after the logshipper is setup? Though it would mean moving runGroup.Add till later too

I considered that, but felt like moving all the runGroup.Add calls later was more error prone 😅 -- we'd have one or two runGroup.Add calls to move, given that we don't add the trace exporter if we can't init it.

@RebeccaMahany RebeccaMahany added the bug-fixes Bug Fixes label Sep 17, 2024
Copy link
Contributor

@James-Pickett James-Pickett left a comment

Choose a reason for hiding this comment

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

nice

@RebeccaMahany
Copy link
Contributor Author

RebeccaMahany commented Sep 17, 2024

I wanted to understand why this was happening, and figured it out. When we set the new log shipping handler, we initialize a new slogger, so the previous reference to the slogger never gets that new handler.

// we have to rebuild the handler everytime because the slogmulti package we're
// using doesn't support adding handlers after the Fanout handler has been created

@RebeccaMahany RebeccaMahany added this pull request to the merge queue Sep 19, 2024
Merged via the queue into kolide:main with commit 385f4fa Sep 19, 2024
31 checks passed
@RebeccaMahany RebeccaMahany deleted the becca/ship-rungroup-logs branch September 19, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fixes Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants