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

Remove retransformation on failed resolution #6423

Merged
merged 1 commit into from
Dec 29, 2023

Conversation

jpbempel
Copy link
Member

@jpbempel jpbempel commented Dec 29, 2023

What Does This Do

Previously, in case of failed resolution from probe id we retransformed the current class to make sure the code is in sync with probe definitions. if a loop was still executing instrumented code, this can lead to a storm of retransformation which can hurt significantly performance of the application (retransform is STW). we now do nothing except log a warning and send it as telemetry.

Motivation

avoid performance issue in rare cases

Additional Notes

Jira ticket: DEBUG-2022

Previously, in case of failed resolution from probe id we
retransformed the current class to make sure the code is in sync with
probe definitions. if a loop was still executing instrumented code,
this can lead to a storm of retransformation which can hurt
significantly performance of the application (retransform is STW).
we now do nothing except log a warning and send it as telemetry.
@jpbempel jpbempel requested a review from a team as a code owner December 29, 2023 10:21
@jpbempel jpbempel requested review from shatzi and removed request for a team December 29, 2023 10:21
@jpbempel jpbempel added the comp: debugger Dynamic Instrumentation label Dec 29, 2023
retransformClasses(Collections.singletonList(callingClass));
return null;
if (definition == null) {
LOGGER.warn(SEND_TELEMETRY, "Cannot resolve probe id=" + encodedProbeId);
Copy link
Contributor

Choose a reason for hiding this comment

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

does LOGGER.warn is sent to instrumentation telemetry? or that would be on a different PR

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it is sent only if DD_TELEMETRY_LOG_COLLECTION_ENABLED is set to true for the moment.
it will be turned to true by default once they assess the volume of log sent with that

@@ -240,15 +242,10 @@ private void storeDebuggerDefinitions(ConfigurationComparer changes) {

// /!\ This is called potentially by multiple threads from the instrumented code /!\
@Override
public ProbeImplementation resolve(String encodedProbeId, Class<?> callingClass) {
public ProbeImplementation resolve(String encodedProbeId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice cleanup.

@jpbempel jpbempel merged commit 3472e15 into master Dec 29, 2023
75 checks passed
@jpbempel jpbempel deleted the jpbempel/fail-resolution branch December 29, 2023 22:06
@github-actions github-actions bot added this to the 1.27.0 milestone Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: debugger Dynamic Instrumentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants