Skip to content
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.

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

Implement support for CRaC #2250

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Implement support for CRaC #2250

wants to merge 12 commits into from

Conversation

jbedell-newrelic
Copy link
Contributor

Several adjustments made:

  • Close .new and .old files after weaving.
  • Implement the CRaC Resource interface on the connection to the collector to close and re-open the connection.
  • Implement the CRaC Resource interface on the logger to close and re-open the file.
  • Added a feature flag to load our log config programmatically rather than from the config file, default is programmatic.
  • This feature will only work after the supporting JDKs have released the fix for https://bugs.openjdk.org/browse/JDK-8349365

@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2025

Codecov Report

Attention: Patch coverage is 22.36842% with 59 lines in your changes missing coverage. Please review.

Project coverage is 70.53%. Comparing base (17a135e) to head (459085b).
Report is 30 commits behind head on main.

Files with missing lines Patch % Lines
...n/java/com/newrelic/agent/logging/Log4jLogger.java 20.68% 23 Missing ⚠️
...va/com/newrelic/agent/logging/Log4jLogManager.java 32.00% 15 Missing and 2 partials ⚠️
...gent/transport/apache/ApacheHttpClientWrapper.java 23.07% 10 Missing ⚠️
...instrumentation/context/FinalClassTransformer.java 0.00% 9 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2250      +/-   ##
============================================
- Coverage     70.57%   70.53%   -0.04%     
  Complexity    10017    10017              
============================================
  Files           842      844       +2     
  Lines         40619    40560      -59     
  Branches       6163     6146      -17     
============================================
- Hits          28668    28611      -57     
+ Misses         9167     9166       -1     
+ Partials       2784     2783       -1     

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

@meiao meiao linked an issue Mar 3, 2025 that may be closed by this pull request
meiao
meiao previously approved these changes Mar 3, 2025
Copy link
Contributor

@meiao meiao left a comment

Choose a reason for hiding this comment

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

I feel funny having the supportability metric in the http client wrapper. It could as well be in the Logger, but it is a responsibility of neither.
Almost feel like we should have a CracService or util that registers itself with Crac for the callbacks, and our listeners would then register on this.
But this is just another abstraction/complexity layer just to keep up with the single responsibility principle.
So I won't hold the review because of this. But I think the other things are worth a look.

meiao
meiao previously approved these changes Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Add support for CRaC to the Java Agent
4 participants