internal/civisibility/integrations/gotesting: fixes for orchestrion autoinstrumentation#2844
Merged
Merged
Conversation
BenchmarksBenchmark execution time: 2024-09-12 09:11:31 Comparing candidate commit 50434cc in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 56 metrics, 1 unstable metrics. scenario:BenchmarkSetTagMetric-24
scenario:BenchmarkSetTagString-24
|
tonyredondo
force-pushed
the
tony/civisibility-orchestrion-fixes
branch
from
September 10, 2024 11:43
f2eb2d2 to
bf4fd9e
Compare
tonyredondo
marked this pull request as ready for review
September 10, 2024 11:43
anmarchenko
approved these changes
Sep 10, 2024
tonyredondo
force-pushed
the
tony/civisibility-orchestrion-fixes
branch
from
September 11, 2024 08:23
83c07e4 to
e32d8bb
Compare
…utoinstrumentation For orchestrion we are going to instrument testing.M.Run(), testing.T.Run() and testing.B.Run() methods, for that we need to make some fixes and add some checks to avoid multiple instrumentation over the same Func
…utoinstrumentation fixes
… for orchestrion autoinstrumentation
…es, modules and sessions.
…d store the civisibility test from `testing.T` and/or `testing.common` both structs implements `testing.TB` we want to get always the same test no matter the struct used.
… if a ddtest has been failed or skipped so we can avoid rewriting existing useful messages by a generic one. The case of Error()/Errorf() calling Fail()/Failf() internally
… and ddTestItem.skipped bool type to an atomic.Int32 to avoid any race condition.
tonyredondo
force-pushed
the
tony/civisibility-orchestrion-fixes
branch
from
September 11, 2024 13:37
c70e3af to
d758e25
Compare
… were caching the instrumented function from an original one, caching also the closure data of the function, now we don't catch the instrumented function. Also, added some tests to handle this scenario.
tonyredondo
force-pushed
the
tony/civisibility-orchestrion-fixes
branch
from
September 11, 2024 14:00
d758e25 to
2246373
Compare
ManuelPalenzuelaDD
approved these changes
Sep 12, 2024
ManuelPalenzuelaDD
left a comment
ManuelPalenzuelaDD
left a comment
Contributor
There was a problem hiding this comment.
LGTM, left a minor comment
darccio
approved these changes
Sep 12, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
For orchestrion we are going to instrument
testing.M.Run(),testing.T.Run()andtesting.B.Run()methods, for that we need to make some fixes and add some checks to avoid multiple instrumentation over the same FuncThis PR also fixes an issue found with orchestrion about Test Suite Level Visibility (tslv) objects having
ParentId != 0this breaks the backend json schema. Now we make sure that structs likeTest,TestSuite,TestModuleandTestSessionnever set the ParentId value (Only normal spans are allowed to have it).Motivation
While working with the orchestrion autoinstrumentation I'm hitting this issues. This PR fixes those issues.
Reviewer's Checklist
Unsure? Have a question? Request a review!