Skip to content

ddtace/tracer: add missing span read locks#3278

Merged
kakkoyun merged 3 commits into
DataDog:mainfrom
jachym-tousek-keboola:fix-span-data-race
Jun 6, 2025
Merged

ddtace/tracer: add missing span read locks#3278
kakkoyun merged 3 commits into
DataDog:mainfrom
jachym-tousek-keboola:fix-span-data-race

Conversation

@jachym-tousek-keboola

@jachym-tousek-keboola jachym-tousek-keboola commented Mar 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a data race bug reported in #3270.

Motivation

Out team saw the data race in our logs.

Fixes #3270

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.
  • For internal contributors, a matching PR should be created to the v2-dev branch and reviewed by @DataDog/apm-go.

Unsure? Have a question? Request a review!

@jachym-tousek-keboola
jachym-tousek-keboola marked this pull request as ready for review March 10, 2025 10:16
@jachym-tousek-keboola
jachym-tousek-keboola requested a review from a team as a code owner March 10, 2025 10:16
@darccio
darccio requested a review from a team March 10, 2025 10:18
@jachym-tousek-keboola jachym-tousek-keboola changed the title ddtace/tracer: add data race test ddtace/tracer: add missing span read locks Mar 10, 2025
@jachym-tousek-keboola

jachym-tousek-keboola commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

Hi, is there anything I should improve about this PR? It's my first time contributing to dd-trace-go so I'm unsure if I did everything that was expected.

@darccio

darccio commented Apr 11, 2025

Copy link
Copy Markdown
Member

@jachym-tousek-keboola Sorry, we are in the middle of the last efforts for releasing our v2. We'll review this once the release is ready.

@kakkoyun
kakkoyun self-requested a review April 28, 2025 08:39
@kakkoyun

kakkoyun commented Apr 28, 2025

Copy link
Copy Markdown
Member

@jachym-tousek-keboola I'm also working on some similar changes but broader. I'll make sure sure I request a review from you.

@jachym-tousek-keboola

jachym-tousek-keboola commented May 12, 2025

Copy link
Copy Markdown
Contributor Author

Sure, ty!

nsrip-dd
nsrip-dd approved these changes May 21, 2025
View reviewed changes

@nsrip-dd nsrip-dd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for your contribution! Fix in general looks good to me. Looks like this has picked up some conflicts, though.

@kakkoyun are you still planning on the changes you mentioned? Do they conflict with this fix, or could we land this in the mean time?

@kakkoyun

kakkoyun commented May 21, 2025

Copy link
Copy Markdown
Member

@kakkoyun are you still planning on the changes you mentioned? Do they conflict with this fix, or could we land this in the mean time?

I still need time for those so let's proceed with this one. Thanks a lot for the contribution.

@kakkoyun

kakkoyun commented May 21, 2025

Copy link
Copy Markdown
Member

@jachym-tousek-keboola We need a rebase though.

@jachym-tousek-keboola

jachym-tousek-keboola commented May 23, 2025

Copy link
Copy Markdown
Contributor Author

@kakkoyun Done. Can you allow the workflow to run to see if everything passes?

@kakkoyun

kakkoyun commented May 28, 2025

Copy link
Copy Markdown
Member

@jachym-tousek-keboola we need to address the CI pointed issues. Let me know if you don't have cycles to push this thtough.

@jachym-tousek-keboola
jachym-tousek-keboola force-pushed the fix-span-data-race branch 2 times, most recently from 84cbeeb to be44626 Compare May 29, 2025 11:07
@jachym-tousek-keboola

jachym-tousek-keboola commented May 29, 2025

Copy link
Copy Markdown
Contributor Author

@kakkoyun I think I fixed the issues. Can you try again please?

nsrip-dd and others added 2 commits May 30, 2025 08:24
@kakkoyun
kakkoyun enabled auto-merge (squash) June 6, 2025 10:15
@kakkoyun
kakkoyun merged commit 2d06fcf into DataDog:main Jun 6, 2025
103 checks passed
@jachym-tousek-keboola
jachym-tousek-keboola deleted the fix-span-data-race branch June 6, 2025 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kakkoyun kakkoyun kakkoyun approved these changes
@nsrip-dd nsrip-dd nsrip-dd approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG]: Data race issue with span

4 participants

@jachym-tousek-keboola @darccio @kakkoyun @nsrip-dd