Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

httptrace: do not mutate shared logger#51826

Merged
bobheadxi merged 2 commits into
mainfrom
httptrace-fix-logging-traces
May 11, 2023
Merged

httptrace: do not mutate shared logger#51826
bobheadxi merged 2 commits into
mainfrom
httptrace-fix-logging-traces

Conversation

@bobheadxi

@bobheadxi bobheadxi commented May 11, 2023

Copy link
Copy Markdown
Member

Previously, every slow request would be logged with the first trace ID that the middleware encounters. This change ensures we make a copy of the base logger first before creating new loggers with additional fields.

Detected by @michaellzc who noticed all slow requests were being logged with the same executor request, even when they weren't :)

Test plan

n/a

@bobheadxi bobheadxi requested review from a team and michaellzc May 11, 2023 17:48
@cla-bot cla-bot Bot added the cla-signed label May 11, 2023
@sourcegraph-bot

sourcegraph-bot commented May 11, 2023

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff d39cf9f...ad1bb70.

Notify File(s)
@keegancsmith internal/trace/httptrace.go
@sourcegraph/dev-experience internal/trace/httptrace.go

@michaellzc michaellzc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🎉

backport?

@bobheadxi bobheadxi merged commit ce84251 into main May 11, 2023
@bobheadxi bobheadxi deleted the httptrace-fix-logging-traces branch May 11, 2023 18:25
@github-actions

Copy link
Copy Markdown
Contributor

The backport to 5.0 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-5.0 5.0
# Navigate to the new working tree
cd .worktrees/backport-5.0
# Create a new branch
git switch --create backport-51826-to-5.0
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ce842515c0a800aea314ef27437d7dfdb2bee79c
# Push it to GitHub
git push --set-upstream origin backport-51826-to-5.0
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-5.0

Then, create a pull request where the base branch is 5.0 and the compare/head branch is backport-51826-to-5.0.

@github-actions github-actions Bot added backports failed-backport-to-5.0 release-blocker Prevents us from releasing: https://about.sourcegraph.com/handbook/engineering/releases labels May 11, 2023
bobheadxi added a commit that referenced this pull request May 11, 2023
Previously, every slow request would be logged with the first trace ID
that the middleware encounters. This change ensures we make a copy of
the base logger first before creating new loggers with additional
fields.

Detected by @michaellzc who noticed all slow requests were being logged
with the same executor request, even when they weren't :)

## Test plan

n/a

(cherry picked from commit ce84251)
coury-clark pushed a commit that referenced this pull request May 11, 2023
Previously, every slow request would be logged with the first trace ID
that the middleware encounters. This change ensures we make a copy of
the base logger first before creating new loggers with additional
fields.

See #51826

## Test plan

n/a

(cherry picked from commit ce84251)
@camdencheek camdencheek added backported-to-5.0 and removed release-blocker Prevents us from releasing: https://about.sourcegraph.com/handbook/engineering/releases failed-backport-to-5.0 labels May 16, 2023
cesrjimenez pushed a commit that referenced this pull request May 17, 2023
Previously, every slow request would be logged with the first trace ID
that the middleware encounters. This change ensures we make a copy of
the base logger first before creating new loggers with additional
fields.

Detected by @michaellzc who noticed all slow requests were being logged
with the same executor request, even when they weren't :)

## Test plan

n/a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants