8000 Defer remote components to avoid OkHttp class-loading side-effects by mcculls · Pull Request #8131 · DataDog/dd-trace-java · GitHub
[go: up one dir, main page]
Skip to content

Defer remote components to avoid OkHttp class-loading side-effects #8131

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

Merged
merged 8 commits into from
Dec 30, 2024
Merged

Defer remote components to avoid OkHttp class-loading side-effects #8131

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
92ca151
Additional logging to help triage custom log-manager / jmx-builder te…
mcculls Dec 28, 2024
40c7583
Move SharedCommunicationObjects creation to InstallDatadogTracerCallb…
mcculls Dec 22, 2024
84d2b6d
Add ability to pause remote components until SharedCommunicationObjec…
mcculls Dec 28, 2024
9b97d7a
Delay starting trace-writer and data-stream-monitoring until remote I…
mcculls Dec 28, 2024
2cdc507
Delay starting logs-intake backend until remote I/O is allowed
mcculls Dec 28, 2024
9a09d98
Install tracer and logs-intake as soon as necessary, but defer any re…
mcculls Dec 28, 2024
9ee929b
Remove exemption where we didn't defer if the custom logging manager
mcculls Dec 28, 2024
c9cf18f
Adjust minimumBranchCoverage to account for new double-checked lock i…
mcculls Dec 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading < 10000 /div>
Diff view
Diff view
Prev Previous commit
Adjust minimumBranchCoverage to account for new double-checked lock i… 
…n SharedCommunicationObjects
  • Loading branch information
@mcculls
mcculls committed Dec 28, 2024
commit c9cf18f1498cbb46fd0f75961ef70d9d85fdfdb0
2 changes: 1 addition & 1 deletion communication/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ dependencies {
}

ext {
minimumBranchCoverage = 0.6
minimumBranchCoverage = 0.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the whole module not testable or it the cost too high?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the double-checked lock that tips things over - attempting to cover all the cases for this piece of code would be very intensive (you'd end up testing the narrow volatile visibility edge.) There are other branches in SharedCommunicationObjects which are not currently tested which is why we're at this boundary to begin with, but adding tests for those unrelated pieces of code in this PR is IMHO confusing for future reviewers.

I could just exclude SharedCommunicationObjects from branch coverage completely, but that feels worse - it's already excluded from instrumentation coverage - so reducing the coverage requirement here by a small amount is the least worst option.

The proper solution is to add tests to increase coverage in a separate PR, as a separate work item.

minimumInstructionCoverage = 0.8
excludedClassesCoverage = [
'datadog.communication.ddagent.ExternalAgentLauncher',
Expand Down
Loading
0