Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add TraceAbleRequestContextStorage for ease of detect context leaks #4232
Add TraceAbleRequestContextStorage for ease of detect context leaks #4232
Changes from 7 commits
1d64827
c79b852
443e740
c431103
06c728e
0f5e5e5
bcf92df
82bb1a8
a7fd563
ea2e675
cb8dcf4
50d4127
818c166
5207a4c
d24e0a2
61e767d
449f051
4dabd01
8909ace
fdd8490
5562cfe
f124fc8
a71a725
c4a534f
3b00c2a
af31a6e
ab4de84
9f681d4
a72a9ca
d43b052
4f8a8f2
6cbcfab
89ddc46
6b4b9f9
083d560
4fb9575
7b3f3d1
9d929bf
90e6e78
a0540c8
9156898
50384a2
0fe21fd
caba3d6
1302481
9e8e10e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is there any reason this class can't be under
internal.common
? (so that this class wouldn't need to bepublic
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, I didn't add Flag
requestContextLeakDetectionSampler
for enabling this feature. I intentionally makeLeakTracingRequestContextStorage
public, so that users can use it to supply viaRequestContextStorageProvider
SPI. Since we already added the flag andRequestContextStorageProvider
SPI will be removed as #4211, I'll move to itinternal.common
. Thank you.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, we can use
Flags.requestContextLeakDetectionSampler()
as the default value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just call
delegate.push(...)
ifsampler == Sampler.never()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
sampler
member ofLeakTracingRequestContextStorage
cannot beSampler.never()
because we already have the conditionsampler == Sampler.never()
inRequestContextUtil
Also, I'll move
LeakTracingRequestContextStorage
tointernal.common
as @jrhee17 mentioned.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. If it is moved to internal, we don't have to double-check it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little concerned that this validation logic is:
armeria/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java
Line 233 in f899257
armeria/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java
Line 236 in f899257
armeria/core/src/main/java/com/linecorp/armeria/common/ThreadLocalRequestContextStorage.java
Line 50 in f899257
e.g.
I guess this PR is a good start, but I'm wondering if it would be more robust to "attach" the caller thread/stacktrace to the
RequestContext
when pushing, and retrieving this information if theRequestContext
ends up in an invalid state.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc. other maintainers @ikhoon @minwoox @trustin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prevContext
or its root is sampled, just like how distributed tracing is propagated to its subspans.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thanks for pointing this out 🙏.
Hi @trustin, Could help elaborate on this option? thanks.
Another option that I can think of is to store a stack of stack traces. Actually, @minwoox mentioned in the issue that stack is needed because context can be pushed multiple times before getting popped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we need a stack to store stack traces. A request context could be pushed multiple times before it's popped, so the stack is the right data structure for this case.
We might be able to do it something like:
Let's suppose this situation:
Because b and c are not popped we need to provide the stack traces of those contexts. However, if b is sampled and c isn't sampled we cannot provide them.
In order to solve this situation, I think we need to make a decision to sample or not only when the root of the context is pushed so that we can have all stack traces of the context flow. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got it, Thanks for the explanation 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. Should we just call
delegate.pop(...)
ifsampler == Sampler.never()
?