HBASE-27153 Improvements to read-path tracing #4572
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ndimiduk
requested review from
Apache9,
apurtell,
huaxiangsun,
joshelser and
taklwu
Member
Author
|
These are some screenshots of traces emitted by a build of branch-2 plus this patch, running in pseudo-distributed mode, sent to a Jaeger backed. Notice that the span events are marked as lines on spans. See the attributes on those event.s
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
apurtell
approved these changes
Jun 23, 2022
Contributor
There was a problem hiding this comment.
I wish we didn't have to pass the scan attribute builder through as an extra parameter to methods that don't care about tracing per se, they simply exist on the traced path. Is it possible to use threadlocals or some otel notion of thread context for these? Or is that an anti-pattern?
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Member
Author
The reason that I have to pass this attributes object around is that I'm providing context that is eventually annotated to a span "event" -- essentially a log line. Otel API lets me stuff arbitrary attributes into a thread local for span attributes, not for span event attributes.
I am also not a fan of the current approach. Introducing a |
ndimiduk
force-pushed
the
27153-trace-read-path
branch
from
74de7bb to
3e73a7e
Compare
Member
Author
|
@apurtell I changed the implementation to use the otel |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
ndimiduk
commented
Jul 1, 2022
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java
Show resolved
Hide resolved
apurtell
approved these changes
Jul 1, 2022
Contributor
|
Approved. I like the style way of building attributes along a method call chain without adding the builder as a method parameter. |
ndimiduk
force-pushed
the
27153-trace-read-path
branch
from
3e73a7e to
25aa61a
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
TestAsyncTableScan.testReverseScanNoStopKey[1: scan=batch] failure seems relevant. |
ndimiduk
force-pushed
the
27153-trace-read-path
branch
from
25aa61a to
38ed920
Compare
Member
Author
This failure is baffling to me. The assertion failed because a span with the name |
Signed-off-by: Andrew Purtell <apurtell@apache.org>
ndimiduk
force-pushed
the
27153-trace-read-path
branch
from
38ed920 to
0bf363e
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Take another pass through tracing of the read path, make adjustments accordingly. One of the major concerns raised previously is that we create a span for every block access. Start by simplifying this to trace events and see what else comes up.
Block-level and stream-level interactions are represented as span events with attached attributes (that's otel terminology for "structured logs", attached to the spans). I made an effort to surface information that an operator might be interested in while diagnosing slow read performance, and details that the previous generation of dev had already seen fit to surface via trace-level logging. We no longer emit spans for
HFileReaderImpl.readBlock. I hope this is a good compromise between visibility and performance.Also added a span context for block-cache pre-fetching.