Skip to content

operator: Attach context to logs when available #39728

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 29, 2025
Merged

operator: Attach context to logs when available #39728

merged 1 commit into from
Jul 29, 2025

Conversation

HadrienPatte
Copy link
Member

Since the migration to slog, it is possible to attach contexts to logs. This is recommended by the slog docs:

It is recommended to pass a context to an output method if one is available.

This allows things like trace IDs to be extracted from contexts and attached to logs, allowing log/trace correlation.

This PR uses the XxxContext() slog methods when a context.Context is available in packages under operator/.

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!
operator: Attach context to logs when available

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 26, 2025
@HadrienPatte
Copy link
Member Author

/test

@HadrienPatte HadrienPatte marked this pull request as ready for review May 26, 2025 15:21
@HadrienPatte HadrienPatte requested review from a team as code owners May 26, 2025 15:21
aanm
aanm requested changes May 26, 2025
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

@HadrienPatte thanks for the PR. The migration to slog is not fully completed. Can we wait until all of these PRs are merged https://github.com/cilium/cilium/pulls?q=is%3Aopen+is%3Apr+author%3Aaanm+label%3Afeature%2Fslog before this one?

I'm planning to create one more PR besides the ones already created and after that the slog migration is done.

Thanks!

@aanm aanm added dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed feature/slog release-note/misc This PR makes changes that have no direct user impact. labels May 26, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 26, 2025
@HadrienPatte
Copy link
Member Author

Sure no problem 👍

HadrienPatte
HadrienPatte commented Jun 5, 2025
View reviewed changes
@aanm aanm self-requested a review June 13, 2025 14:39
@aanm aanm removed the dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed label Jun 13, 2025
@HadrienPatte HadrienPatte force-pushed the operator-slog-context branch from cba49fd to 808e4f2 Compare June 13, 2025 15:46
@HadrienPatte HadrienPatte force-pushed the operator-slog-context branch from 808e4f2 to 0a0e680 Compare June 17, 2025 22:39
pippolo84
pippolo84 approved these changes Jun 18, 2025
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

Thanks! ✅

@joestringer
Copy link
Member

@aanm @HadrienPatte are there any implications on the v1.18 release to either include or exclude this PR? If this is important for the slog migration then we might want to track it as a release blocker and add the release-blocker/1.18 label to this PR (and follow up in the coming weeks to get it rebased & merged)

@HadrienPatte
Copy link
Member Author

I don't think we need this PR in 1.18 as it's only bringing context logging to a small subset of the repository, a couple more followup PR will be necessary to cover the whole repo

@HadrienPatte HadrienPatte added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Jun 23, 2025
@aanm
Copy link
Member

aanm commented Jun 24, 2025

/test

@joestringer joestringer removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Jul 1, 2025
@HadrienPatte
operator: Attach context to logs when available
1aa29e5 
Since the migration to `slog`, it is possible to attach contexts to
logs. This is recommended by the [`slog` docs](https://pkg.go.dev/log/slog#hdr-Contexts):

> It is recommended to pass a context to an output method if one is available.

This allows things like trace IDs to be extracted from contexts and
attached to logs, allowing log/trace correlation.

This PR uses the `XxxContext()` `slog` methods when a `context.Context`
is available in packages under `operator/`.

Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
@HadrienPatte HadrienPatte force-pushed the operator-slog-context branch from 0a0e680 to 1aa29e5 Compare July 9, 2025 08:51
@jrajahalme
Copy link
Member

/test

@joestringer joestringer enabled auto-merge July 28, 2025 18:33
@joestringer joestringer added this pull request to the merge queue Jul 29, 2025
Merged via the queue into cilium:main with commit 22cdf36 Jul 29, 2025
68 checks passed
@HadrienPatte HadrienPatte deleted the operator-slog-context branch July 29, 2025 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derailed derailed derailed approved these changes

@pippolo84 pippolo84 pippolo84 approved these changes

@ysksuzuki ysksuzuki ysksuzuki approved these changes

@aanm aanm aanm approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

@rastislavs rastislavs rastislavs approved these changes

@thorn3r thorn3r thorn3r approved these changes

@sayboras sayboras Awaiting requested review from sayboras sayboras is a code owner automatically assigned from cilium/envoy

+1 more reviewer

@doniacld doniacld doniacld approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
feature/slog release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@HadrienPatte @joestringer @aanm @jrajahalme @derailed @pippolo84 @ysksuzuki @rastislavs @doniacld @thorn3r