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

Jump to bottom

Do not record wire timestamp when no request was transferred #1912

Merged
merged 1 commit into from Jul 22, 2019
Merged

Do not record wire timestamp when no request was transferred #1912

merged 1 commit into from Jul 22, 2019

Conversation

trustin
Copy link
Collaborator

@trustin trustin commented Jul 18, 2019

Motivation:

On the client side, it is possible that a request is not sent to the
wire, e.g. when connection error or validation failure occurs.

Modifications:

  • Make sure REQUEST_FIRST_BYTES_TRANSFERRED is available before
    logging wire receive.

Result:

@trustin trustin added the defect label Jul 18, 2019
@trustin trustin added this to the 0.89.0 milestone Jul 18, 2019
@trustin
Do not record wire timestamp when no request was transferred
36f9959 
Motivation:

On the client side, it is possible that a request is not sent to the
wire, e.g. when connection error or validation failure occurs.

Modifications:

- Make sure `REQUEST_FIRST_BYTES_TRANSFERRED` is available before
  logging wire receive.

Result:

- No more `RequestLogAvailabilityException` in `BraveClient` and
  `HttpTracingClient`.
- Fixes line#1911
@trustin trustin force-pushed the fix_availability_exception branch from 8e07ae6 to 36f9959 Compare July 18, 2019 09:30
minwoox
minwoox reviewed Jul 18, 2019
View reviewed changes
brave/src/main/java/com/linecorp/armeria/client/brave/BraveClient.java
// The request might have failed even before it's sent, e.g. validation failure, connection error.
if (log.isAvailable(RequestLogAvailability.REQUEST_FIRST_BYTES_TRANSFERRED)) {
SpanTags.logWireSend(span, log.requestFirstBytesTransferredTimeNanos(), log);
}
Copy link
Member

@minwoox minwoox Jul 18, 2019 •
edited

Choose a reason for hiding this comment

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

Shouldn't we do something like:

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, 😓 Misread the code. TT

ikhoon
ikhoon approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@codecov
Copy link

codecov bot commented Jul 18, 2019

Codecov Report

Merging #1912 into master will decrease coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1912      +/-   ##
============================================
- Coverage     73.28%   73.26%   -0.02%     
+ Complexity     8980     8976       -4     
============================================
  Files           794      794              
  Lines         35170    35172       +2     
  Branches       4322     4322              
============================================
- Hits          25774    25769       -5     
+ Misses         7192     7190       -2     
- Partials       2204     2213       +9
Impacted Files Coverage Δ Complexity Δ
...corp/armeria/client/tracing/HttpTracingClient.java 90.38% <50%> (-1.78%) 13 <2> (ø)
...com/linecorp/armeria/client/brave/BraveClient.java 90% <50%> (-1.84%) 13 <2> (ø)
...lient/circuitbreaker/CircuitBreakerHttpClient.java 69.69% <0%> (-3.04%) 10% <0%> (-1%)
...a/common/grpc/protocol/ArmeriaMessageDeframer.java 70.33% <0%> (-1.92%) 46% <0%> (-3%)
...inecorp/armeria/server/grpc/ArmeriaServerCall.java 86.61% <0%> (-0.4%) 81% <0%> (-1%)
.../com/linecorp/armeria/server/docs/ServiceInfo.java 73.17% <0%> (+2.43%) 17% <0%> (+1%) ⬆️

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 47624dc...36f9959. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 18, 2019 •
edited

Codecov Report

Merging #1912 into master will decrease coverage by 0.04%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1912      +/-   ##
============================================
- Coverage     73.28%   73.24%   -0.05%     
+ Complexity     8980     8974       -6     
============================================
  Files           794      794              
  Lines         35170    35172       +2     
  Branches       4322     4322              
============================================
- Hits          25774    25761      -13     
- Misses         7192     7201       +9     
- Partials       2204     2210       +6
Impacted Files Coverage Δ Complexity Δ
...corp/armeria/client/tracing/HttpTracingClient.java 90.38% <50%> (-1.78%) 13 <2> (ø)
...com/linecorp/armeria/client/brave/BraveClient.java 90% <50%> (-1.84%) 13 <2> (ø)
...lient/circuitbreaker/CircuitBreakerHttpClient.java 69.69% <0%> (-3.04%) 10% <0%> (-1%)
...om/linecorp/armeria/server/jetty/JettyService.java 63.05% <0%> (-2.96%) 23% <0%> (-1%)
...corp/armeria/common/stream/FixedStreamMessage.java 89.28% <0%> (-1.79%) 23% <0%> (-1%)
...inecorp/armeria/client/grpc/ArmeriaClientCall.java 80.74% <0%> (-1.49%) 36% <0%> (-1%)
...com/linecorp/armeria/server/HttpServerHandler.java 80.4% <0%> (-0.68%) 79% <0%> (-1%)
...inecorp/armeria/server/grpc/ArmeriaServerCall.java 86.61% <0%> (-0.4%) 81% <0%> (-1%)

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 47624dc...36f9959. Read the comment docs.

@trustin trustin merged commit ae3b5d4 into line:master Jul 22, 2019
@trustin trustin deleted the fix_availability_exception branch July 22, 2019 03:19
@trustin
Copy link
Collaborator Author

trustin commented Jul 22, 2019

Thanks for reviewing. 😄

fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
@trustin
Do not record wire timestamp when no request was transferred (line#1912)
b236186 
Motivation:

On the client side, it is possible that a request is not sent to the
wire, e.g. when connection error or validation failure occurs.

Modifications:

- Make sure `REQUEST_FIRST_BYTES_TRANSFERRED` is available before
  logging wire receive.

Result:

- No more `RequestLogAvailabilityException` in `BraveClient` and
  `HttpTracingClient`.
- Fixes line#1911
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikhoon ikhoon ikhoon approved these changes

@minwoox minwoox minwoox approved these changes

Assignees
No one assigned
Labels
defect
Projects
None yet
Milestone
0.89.0
Development

Successfully merging this pull request may close these issues.

RequestLogAvailabilityException: REQUEST_FIRST_BYTES_TRANSFERRED when RequestLogAvailability is COMPLETE
3 participants
@trustin @ikhoon @minwoox