Skip to content

Add collector connection TTL and recovery tests - #3081

Open
PongPong wants to merge 6 commits into
newrelic:mainfrom
PongPong:feat/recover-conn
Open

Add collector connection TTL and recovery tests#3081
PongPong wants to merge 6 commits into
newrelic:mainfrom
PongPong:feat/recover-conn

Conversation

@PongPong

Copy link
Copy Markdown

Before contributing, please read our contributing guidelines and code of conduct.

Overview

Adds a configurable maximum lifetime for pooled collector connections to prevent reuse of connections that have been silently terminated by an intermediary such as a proxy, firewall, or load balancer.

The new collector_connection_ttl setting:

  • Accepts a non-negative number of seconds.
  • Defaults to 0, preserving the existing unlimited connection lifetime.
  • Can be configured through newrelic.yml, the newrelic.config.collector_connection_ttl system property, or the NEW_RELIC_COLLECTOR_CONNECTION_TTL environment variable.
  • Applies only when leasing pooled connections and does not interrupt active requests.

The PR also adds parameterized connection-lifecycle and recovery tests:

  • Embedded Jetty covers HTTP and TLS keep-alive reuse, idle connection termination, explicit connection closure, active request abortion, long-running requests, and repeated recovery.
  • Toxiproxy covers TCP resets, blackholes, latency, and truncated responses.
  • Toxiproxy tests automatically detect Docker and can be explicitly disabled.

Related GitHub Issue

N/A

Testing

The following focused test suites were run locally:

  • Embedded Jetty connection-lifecycle tests: 11 passed.
  • Toxiproxy network-fault tests: 4 passed.
  • Apache HTTP client TTL and CRaC tests: 8 passed.
  • Collector connection TTL configuration tests: 8 passed.
  • Toxiproxy opt-out behavior: 4 tests skipped successfully.
  • git diff --check passed.

The complete repository test suite was not run locally. GitHub Actions will run the broader suite when the pull request is opened.

Checks

  • Your contributions are backwards compatible with relevant frameworks and APIs.
  • Your code does not contain any breaking changes. Otherwise please describe.
  • Your code does not introduce any new dependencies. Otherwise please describe.

This PR adds Testcontainers 1.21.3 and its Toxiproxy module as test-only dependencies. They are used exclusively for TCP fault-injection integration tests and are not included in the Java agent runtime or distribution. Jetty was already present as a test dependency.

Signed-off-by: Pong Lam <pg051285@gmail.com>
@CLAassistant

CLAassistant commented Aug 27, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@jtduffy jtduffy moved this from Triage to In Sprint in Java Engineering Board Sep 1, 2026
@jtduffy jtduffy self-assigned this Sep 1, 2026
@codecov-commenter

codecov-commenter commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.62500% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.98%. Comparing base (5181cbe) to head (be49829).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...gent/transport/apache/ApacheHttpClientWrapper.java 57.14% 3 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##               main    #3081       +/-   ##
=============================================
+ Coverage     37.95%   70.98%   +33.02%     
- Complexity     5532    10853     +5321     
=============================================
  Files           873      873               
  Lines         43135    43406      +271     
  Branches       6526     6572       +46     
=============================================
+ Hits          16373    30811    +14438     
+ Misses        24482     9633    -14849     
- Partials       2280     2962      +682     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jtduffy jtduffy 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.

Thank you for the PR. I've requested some changes below.

Comment thread newrelic-agent/src/main/java/com/newrelic/agent/config/DataSenderConfig.java Outdated
Comment thread newrelic-agent/src/main/resources/newrelic.yml Outdated
Comment thread README.md Outdated
Signed-off-by: Pong Lam <pg051285@gmail.com>
Signed-off-by: Pong Lam <pg051285@gmail.com>
@jtduffy jtduffy moved this from In Sprint to Needs Review in Java Engineering Board Sep 3, 2026
@PongPong

PongPong commented Sep 4, 2026

Copy link
Copy Markdown
Author

Follow-up on the remaining failed check: aits / client/http4s.py is also failing on main at 287a3ab with the same test_http4s_ember_client assertion (Expected {count: 2}, observed one matching transaction metric): https://github.com/newrelic/newrelic-java-agent/actions/runs/33775746891/job/100718354977

The corresponding failure for this PR commit is here: https://github.com/newrelic/newrelic-java-agent/actions/runs/33777015267/job/100782407867

The HTTP4s AIT exercises the separate HTTP4s Ember client instrumentation; this PR changes the collector transport based on Apache HttpClient. The unit, functional, and instrumentation matrices—including Java 11 nonForkedTests—are now passing. Based on the identical baseline failure on main, this remaining red check does not appear to have been introduced by the collector connection TTL change. Could the HTTP4s job be rerun or treated as a known baseline failure?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

4 participants