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

Fix log template evaluation #4623

Merged
merged 1 commit into from
Jan 30, 2023
Merged

Conversation

jpbempel
Copy link
Member

What Does This Do

ValueScript is storing result value in a field which can be overwritten by another execution. Now the execute method returns directly the result value and ValueScript became effectively stateless regarding execution.

Motivation

Bug in log template evaluation for concurrent requests

Additional Notes

ValueScript is storing result value in a field which can be
overwritten by another execution. Now the execute method returns
directly the result value and ValueScript became effectively stateless
regarding execution.
@jpbempel jpbempel requested a review from a team as a code owner January 30, 2023 16:20
@jpbempel jpbempel requested review from ojung and removed request for a team January 30, 2023 16:20
@@ -47,26 +48,10 @@ public void addStack(List<CapturedStackFrame> stack) {}

@Override
public String build() {
Copy link
Contributor

Choose a reason for hiding this comment

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

find this method name now confusing... with this is not getMessage and executeExpressions is build?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is one implementation of SummaryBuilder with the original idea that we need to gather all infos about the snapshot before building the summary, hence the builder.
With LogTemplate we move the building of the summary at context adding.
If we get rid of the SnapshotSummaryBuilder we can revisit it :)

Copy link
Member

@ojung ojung left a comment

Choose a reason for hiding this comment

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

👍

@jpbempel jpbempel merged commit 8e35845 into master Jan 30, 2023
@jpbempel jpbempel deleted the jpbempel/fix-valuescript-result branch January 30, 2023 21:21
@jpbempel jpbempel added the comp: debugger Dynamic Instrumentation label Jan 30, 2023
@github-actions github-actions bot added this to the 1.7.0 milestone Jan 30, 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.

3 participants