Add Read Metrics #18160
Add Read Metrics #18160
Conversation
dora/core/client/fs/src/main/java/alluxio/client/file/PositionReadFileInStream.java
Outdated
Show resolved
Hide resolved
dora/core/client/fs/src/main/java/alluxio/client/file/PositionReadFileInStream.java
Outdated
Show resolved
Hide resolved
dora/core/client/fs/src/main/java/alluxio/client/file/ufs/UfsFileInStream.java
Outdated
Show resolved
Hide resolved
dora/core/client/fs/src/main/java/alluxio/client/file/PositionReadFileInStream.java
Outdated
Show resolved
Hide resolved
| return read(byteBuffer.array(), off, len); | ||
| totalBytesRead = read(byteBuffer.array(), off, len); | ||
| if (totalBytesRead > 0) { | ||
| Metrics.BYTES_READ_UFS.inc(totalBytesRead); |
There was a problem hiding this comment.
Instead of adding the metric accounting logic in one specific class, I'd recommend making this logic a dedicated wrapper, e.g. MetricAccountingInputStream, which takes in an underlying generic InputStream, and the metric key that the number of bytes read from the underlying stream would be attributed to. The read methods then delegate to the underlying stream, and adjust the metric according to the bytes read from it.
This approach has the advantage of
- the metric accounting logic is separated from the main IO path, reducing the noise in code.
- the metric accounting logic is pluggable and composable. You can use any combination of input stream and metric key. The metric key will not be tied to a particular class like
UfsFileInStream. If some one creates a new streamWorkerFirstThenFallbackToUfsInputStreamand forgets about plugging in the metric accounting, then you lose track of the bytes read from UFS there.
There was a problem hiding this comment.
UfsFileInStream is the only top-level FileInStream that is created and used in UfsBaseFileSystem.
Various UFS will be opened in the UfsFileInStream.
I agree with you that your suggestion is a good idea. But let's do it later.
Signed-off-by: Huang Hua <huanghua78@msn.com>
huanghua78
force-pushed
the
ReadMetrics
branch
from
fae6ea6
to
0ad24c6
Compare
Signed-off-by: Huang Hua <huanghua78@msn.com>
|
@dbw9580 please review again. |
|
alluxio-bot, merge this please |
|
merge failed: |
|
alluxio-bot, merge this please |
What changes are proposed in this pull request?
Add client read metrics
Why are the changes needed?
These metrics are helpful.
Does this PR introduce any user facing changes?
N/A