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

Provide a way to access a parent log from a child log #2599

Merged
merged 4 commits into from
Mar 19, 2020

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Mar 17, 2020

Motivation:

RequestId is used for tracking the request and the response pair.
A request can be executed multiple times by RetryingClient.
It is difficult to group and trace all retried requests without the request ID of the original request.
By accessing the parent log from the current request context, we are able to log the parent and current request ID pair.

Modifications:

  • Add parent() to RequestLogAccess
  • Include the request ID of parent log to toString() in DefaultClientRequestContext

Results:

Motivation:

`RequestId` is used for tracking the request and the response pair.
A request can be executed multiple times by `RetryingClient`.
It is difficult to group and trace all retried requests without the request ID of the original request.
By accessing the parent log from the current request context, we are able to log the parent and current request ID pair.

Modifications:

- Add `parent()` to `RequestLogAccess`
- Include the request ID of parent to `toString()`

Results:

- You can now access a parent log from a child log.
- `DefaultClientRequest.toString()` contains a parent request ID if available.
- Fixes line#2588
@ikhoon ikhoon added this to the 0.99.0 milestone Mar 17, 2020
@ikhoon ikhoon requested review from minwoox and trustin as code owners March 17, 2020 17:15
Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

LGTM once @trustin's comments are addressed.

@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

Merging #2599 into master will decrease coverage by 0.02%.
The diff coverage is 76.92%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2599      +/-   ##
============================================
- Coverage     73.34%   73.32%   -0.03%     
+ Complexity    11049    11043       -6     
============================================
  Files           970      971       +1     
  Lines         42523    42527       +4     
  Branches       5294     5300       +6     
============================================
- Hits          31188    31181       -7     
- Misses         8625     8633       +8     
- Partials       2710     2713       +3     
Impacted Files Coverage Δ Complexity Δ
...ecorp/armeria/common/logging/RequestLogAccess.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...corp/armeria/common/logging/DefaultRequestLog.java 75.84% <40.00%> (-0.26%) 207.00 <1.00> (+1.00) ⬇️
...rp/armeria/client/DefaultClientRequestContext.java 92.89% <100.00%> (+0.69%) 74.00 <0.00> (-1.00) ⬆️
...linecorp/armeria/server/ServiceRequestContext.java 73.07% <0.00%> (-21.37%) 21.00% <0.00%> (+2.00%) ⬇️
.../linecorp/armeria/client/ClientRequestContext.java 70.00% <0.00%> (-21.18%) 21.00% <0.00%> (+2.00%) ⬇️
...inecorp/armeria/common/ClosedSessionException.java 54.54% <0.00%> (-18.19%) 5.00% <0.00%> (-1.00%)
...inecorp/armeria/server/HttpResponseSubscriber.java 76.88% <0.00%> (-1.89%) 52.00% <0.00%> (-4.00%)
...a/com/linecorp/armeria/client/HttpChannelPool.java 78.41% <0.00%> (-1.80%) 68.00% <0.00%> (-2.00%)
.../com/linecorp/armeria/server/docs/ServiceInfo.java 78.94% <0.00%> (-1.76%) 20.00% <0.00%> (-1.00%)
...p/armeria/server/DefaultServiceRequestContext.java 85.51% <0.00%> (-1.11%) 50.00% <0.00%> (-6.00%)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d499c6...e7f65bd. Read the comment docs.

@trustin trustin merged commit e0445cf into line:master Mar 19, 2020
@trustin
Copy link
Member

trustin commented Mar 19, 2020

Thanks a lot, @ikhoon !

@ikhoon
Copy link
Contributor Author

ikhoon commented Mar 19, 2020

Thanks for reviewing!

@ikhoon ikhoon deleted the cctx-parent-id branch March 25, 2020 10:45
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
Motivation:

`RequestId` is used for tracking the request and the response pair.
A request can be executed multiple times by `RetryingClient`.
It is difficult to group and trace all retried requests without the request ID of the original request.
By accessing the parent log from the current request context, we are able to log the parent and current request ID pair.

Modifications:

- Add `parent()` to `RequestLogAccess`
- Include the request ID of parent to `toString()`

Results:

- You can now access a parent log from a child log.
- `DefaultClientRequest.toString()` contains a parent request ID if available.
- Fixes line#2588
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to get RequestId of the parent ctx while retrying
3 participants