Skip to content

Conversation

@jbewing
Copy link
Contributor

@jbewing jbewing commented Mar 26, 2025

What

This PR updates expensive tracing calls if tracing is disabled. In particular, the expensive tracing calls as determined by a flamegraph are in BlockIOUtils & HFileBlock. I've audited for other places where tracing is used to eliminate any other expensive calls as well (but couldn't find any outside of BlockIOUtils & HFileBlock).

This generally just follows the pattern of refactoring to use empty Attributes if we can determine that the Span isn't going to be persisted/saved anywhere via Span.isRecordingEnabled e.g. the same approach taken in #6642 / HBASE-29099

How

I used the Span.isRecordingEnabled API (OpenTelemetry Docs). Based on the following docsnippet, I believe that this will work if OpenTelemetry isn't enabled for HBase:

This flag SHOULD be used to avoid expensive computations of a Span attributes or events in case when a Span is definitely not recorded. Note that any child span’s recording is determined independently from the value of this flag (typically based on the sampled flag of a TraceFlags on SpanContext).

HBASE-29222

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 32s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+0 🆗 codespell 0m 0s codespell was not available.
+0 🆗 detsecrets 0m 0s detect-secrets was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
_ master Compile Tests _
+0 🆗 mvndep 0m 12s Maven dependency ordering for branch
+1 💚 mvninstall 3m 38s master passed
+1 💚 compile 4m 46s master passed
+1 💚 checkstyle 0m 55s master passed
+1 💚 spotbugs 2m 33s master passed
+1 💚 spotless 0m 53s branch has no errors when running spotless:check.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 11s Maven dependency ordering for patch
+1 💚 mvninstall 3m 44s the patch passed
+1 💚 compile 4m 15s the patch passed
+1 💚 javac 4m 15s the patch passed
+1 💚 blanks 0m 0s The patch has no blanks issues.
+1 💚 checkstyle 1m 3s the patch passed
+1 💚 spotbugs 2m 38s the patch passed
+1 💚 hadoopcheck 13m 34s Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚 spotless 0m 49s patch has no errors when running spotless:check.
_ Other Tests _
+1 💚 asflicense 0m 20s The patch does not generate ASF License warnings.
48m 22s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6863/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #6863
Optional Tests dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname Linux 583d747d4af2 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / 4bd2012
Default Java Eclipse Adoptium-17.0.11+9
Max. process+thread count 85 (vs. ulimit of 30000)
modules C: hbase-common hbase-server U: .
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6863/1/console
versions git=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered by Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 44s Docker mode activated.
-0 ⚠️ yetus 0m 3s Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+0 🆗 mvndep 0m 14s Maven dependency ordering for branch
+1 💚 mvninstall 4m 14s master passed
+1 💚 compile 1m 41s master passed
+1 💚 javadoc 0m 56s master passed
+1 💚 shadedjars 7m 14s branch has no errors when building our shaded downstream artifacts.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 15s Maven dependency ordering for patch
+1 💚 mvninstall 4m 2s the patch passed
+1 💚 compile 1m 35s the patch passed
+1 💚 javac 1m 35s the patch passed
+1 💚 javadoc 0m 58s the patch passed
+1 💚 shadedjars 7m 13s patch has no errors when building our shaded downstream artifacts.
_ Other Tests _
+1 💚 unit 2m 50s hbase-common in the patch passed.
+1 💚 unit 254m 13s hbase-server in the patch passed.
292m 14s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6863/1/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR #6863
Optional Tests javac javadoc unit compile shadedjars
uname Linux 8ea9c38a6742 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / 4bd2012
Default Java Eclipse Adoptium-17.0.11+9
Test Results https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6863/1/testReport/
Max. process+thread count 5321 (vs. ulimit of 30000)
modules C: hbase-common hbase-server U: .
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6863/1/console
versions git=2.34.1 maven=3.9.8
Powered by Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@Apache9 Apache9 requested review from Copilot and ndimiduk May 22, 2025 14:05
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes tracing by skipping the construction of expensive OpenTelemetry attributes when spans are not recorded.

  • Introduce helper methods to build attributes only when span.isRecording() is true.
  • Refactor existing span.addEvent calls in HFileBlock and BlockIOUtils to use these helpers.
  • Add Javadoc for the new helper methods.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java Added getReadDataBlockInternalAttributes and replaced inline attribute building in readBlockDataInternal with calls to the helper.
hbase-common/src/main/java/org/apache/hadoop/hbase/io/util/BlockIOUtils.java Introduced getDirectAndHeapBytesReadAttributes and getHeapBytesReadAttributes, and refactored multiple span.addEvent calls to use these helpers.
Comments suppressed due to low confidence (3)

hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:2208

  • Add unit tests for getReadDataBlockInternalAttributes to verify that attributes are correctly built when span.isRecording() is true and that no attributes are added when it is false.
private static Attributes getReadDataBlockInternalAttributes(Span span) {

hbase-common/src/main/java/org/apache/hadoop/hbase/io/util/BlockIOUtils.java:408

  • Introduce tests for getDirectAndHeapBytesReadAttributes to ensure that bytes-read attributes are correctly recorded only when span.isRecording() is enabled.
private static Attributes getDirectAndHeapBytesReadAttributes(Span span, int directBytesRead, int heapBytesRead) {

hbase-common/src/main/java/org/apache/hadoop/hbase/io/util/BlockIOUtils.java:430

  • Add unit tests for getHeapBytesReadAttributes to cover both enabled and disabled tracing scenarios.
private static Attributes getHeapBytesReadAttributes(Span span, int heapBytesRead) {

Copy link
Member

@ndimiduk ndimiduk left a comment

Choose a reason for hiding this comment

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

This looks good. I think there's more places where this approach is applicable. Please take those up if you're so inclined!

@ndimiduk ndimiduk merged commit 17ebfbe into apache:master May 23, 2025
1 check passed
ndimiduk pushed a commit to ndimiduk/hbase that referenced this pull request May 23, 2025
ndimiduk pushed a commit to ndimiduk/hbase that referenced this pull request May 23, 2025
ndimiduk pushed a commit to ndimiduk/hbase that referenced this pull request May 23, 2025
ndimiduk pushed a commit that referenced this pull request May 26, 2025
ndimiduk pushed a commit that referenced this pull request May 26, 2025
ndimiduk pushed a commit that referenced this pull request May 26, 2025
mokai87 pushed a commit to mokai87/hbase that referenced this pull request Aug 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants