[FLINK-39546][s3] Improve observability in flink-s3-fs-native by exposing operation-level S3 metrics - #28427
Conversation
gaborgsomogyi
left a comment
There was a problem hiding this comment.
Thanks for the efforts! Left some high level questions
Izeren
left a comment
There was a problem hiding this comment.
Hi @Samrat002, thank you for the change. I have left clarification questions.
Overall, it looks good, but I would like you to extract more cloud agnostic code from native FS. This way, the shims we will need to do to repeat this in Azure, GCS will be really thin. Another concern I have is regrading duplicating non-trivial code to avoid dependency footprint. It seems to me that would be worth extracting lightweight dependency rather than duplicating the code.
ff9f59f to
f027133
Compare
Izeren
left a comment
There was a problem hiding this comment.
Hi @Samrat002, I went through some of it today, and I can see that there are many comments that I have left before that were not replied. Could you please provide replies in threads before I take a second pass.
f027133 to
7bc4048
Compare
288c21d to
540295f
Compare
Izeren
left a comment
There was a problem hiding this comment.
Thank you @Samrat002, high level LGTM. But I would like some brief explanation regarding backwards compatibility of the change:
-import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+import org.apache.flink.metrics.SlidingWindowHistogram;
My original suggestion was rather "move" DescriptiveStatistics to metrics-core and make both runtime-core and native-fs import it from metrics-core (which is supposedly lighter dependency to take).
In current implementation, there is a behavioural change of DescriptiveStatisticsHistogram + we still bring in custom implementation which is otherwise unjustified.
|
Thanks @Izeren . I've decoupled this. DescriptiveStatisticsHistogram is now reverted to its original commons-math3 implementation, so there's no behavioural or backwards-compat change to the runtime histogram. On reusing it from metrics-core, the blocker is that DescriptiveStatisticsHistogramStatistics pulls in commons-math3 (Percentile, StandardDeviation, …). Moving it to flink-metrics-core would force commons-math3 onto the leanest, most widely-depended module (every reporter/connector), which felt like a worse trade than a small dependency-free histogram. And since the S3 plugin only depends on flink-runtime in test scope, its main code can't reference DescriptiveStatisticsHistogram anyway. I'd propose doing that as its own JIRA with proper percentile-compat testing rather than as a side effect here. WDUT? |
540295f to
998b32b
Compare
Thank you @Samrat002, I agree, lets do improvement of histogram implementation separately. This PR is big enough |
998b32b to
e5c8bce
Compare
|
@gaborgsomogyi PTAL whenever time |
|
|
||
| /** Registers normalized filesystem operation metrics supplied by cloud-specific adapters. */ | ||
| @Internal | ||
| public final class FileSystemMetricRecorder { |
There was a problem hiding this comment.
I think there is a global design issue here. This code part is in core and assumes that all FS plugins are having the exact same metrics set. This will never be true. Just to give an example this maps HTTP status codes which assumes all filesystems are HTTP based. Instead a more dynamic approach need to be chosen which is not doing any prediction how a filesystem works.
There was a problem hiding this comment.
I have moved the fixed metric schema and recorder out of flink-core into flink-s3-fs-native. flink-core now only provides the MetricGroup attachment and makes no assumptions about metric names, labels, protocols, status classes, or retry reasons.
The S3 plugin owns its allowlist defaults, iops dependency, and AWS HTTP/retry classification.
9c8a94e to
b70925d
Compare
|
TaskManager never records S3 filesystem metrics —
// Attach file system metrics before HA/blob services can create cached clients.
FileSystem.attachMetrics(processMetricGroup);
Reproduced on a live deployment: TM log shows the S3 filesystem created at Suggest applying the same fix pattern to |
b70925d to
576d616
Compare
|
Now that the TM fix makes publisher attachment order-independent, is |
08b1c19 to
06be5ea
Compare
|
I think my last comment was not well formed. Now we have 3 places to solve have metrics in s3 issue:
This is kind of a brainsplit and I assume that we can drop JM reordering, right? |
Yes, the split-brain behaviour existed because the JM relied on startup reordering while the TM relied on late bridge attachment. I have removed the JM-specific reordering and restored ClusterEntrypoint’s original initialisation order. JM and TM now follow the same lifecycle: each calls FileSystem.attachMetrics() when its process metric group becomes available. The two runtime call sites only provide their respective metric groups. The shared AwsSdkMetricBridge is now the single mechanism responsible for handling initialisation order and cached S3 clients:
Therefore, there are still separate JM and TM attachment points, but no separate correctness strategies: both use the same order-independent bridge behaviour. |
f052a1f to
8b32538
Compare
gaborgsomogyi
left a comment
There was a problem hiding this comment.
LGTM. I need some testing before merge to be sure
8b32538 to
5802226
Compare
|
CI failure is related to https://issues.apache.org/jira/browse/FLINK-40074 |
|
@flinkbot run azure |
…sing operation-level S3 metrics
5802226 to
7cae5fe
Compare
What is the purpose of the change
flink-s3-fs-nativecurrently emits no metrics. When a job's checkpoints, savepoints, or sinks go through it, operators have no visibility into how Flink is actually talking to S3: request volume, latency, throttling, or retries, which makes diagnosing slow or failing checkpoints largely guesswork.This change makes the native S3 filesystem report operation-level S3 metrics into Flink's metric system. It does so by bridging the AWS SDK's built-in metrics SPI into Flink
Counter/Histograminstruments, so every completed S3 API call is counted, timed, and classified.More details on : FLIP-576
Brief change log
flink-core
MetricsAware(@PublicEvolving,org.apache.flink.core.plugin): aFileSystemFactoryimplements it to be handed aMetricGroup.FileSystem.attachMetrics(MetricGroup)(@Internal): creates afilesystemchild group and forwards it to every registeredMetricsAwarefactory; resilient to a misbehaving factory and idempotent.PluginFileSystemFactorynow implementsMetricsAwareand forwardssetMetricGroupto the wrapped inner factory under the plugin classloader. Without this, plugin-loaded filesystems (the normal deployment mode) would silently never receive the group.flink-runtime
ClusterEntrypoint(JobManager) andTaskManagerRunner(TaskManager) callFileSystem.attachMetrics(processMetricGroup)during startup. TheClusterEntrypointservice-init order was adjusted so this runs before HA/blob services cache filesystem clients, otherwise those early clients would be created without a metric group.flink-s3-fs-native
NativeS3FileSystemFactory/NativeS3AFileSystemFactoryimplementMetricsAwareand tag metrics with afilesystem_typelabel set to the scheme (s3vss3a), so the two stay distinguishable.AwsSdkMetricBridgeimplementssoftware.amazon.awssdk.metrics.MetricPublisherand translates eachMetricCollectioninto Flink metrics:api_call_count(labels:op,status_class),api_call_duration_ms(histogram, labelop),throttle_count(labelop),retry_count(labels:op,reason).S3MetricHistogram: a bounded sliding-window histogram backing the duration metric.S3ClientProviderregisters the publisher on the sync/async clients.s3.metrics.enabled(off by default),s3.metrics.allowlist,s3.metrics.histogram.window-size.Verifying this change
This change added tests and can be verified as follows.
Automated tests
AwsSdkMetricBridgeTest— translation of SDK records to Flink metrics;status_classclassification (2xx/4xx/5xx/throttled); retry attribution; allowlist behavior (explicit list,*wildcard, empty → defaults).S3MetricHistogramTest— sliding-window statistics.NativeS3FileSystemFactoryMetricsTest— thefilesystem_typelabel resolves tos3/s3aper factory.FileSystemAttachMetricsTest(flink-core) —attachMetricsunwrapsPluginFileSystemFactoryto reach the real factory, skips non-MetricsAwarefactories, survives a throwing factory, and is idempotent.NativeS3MetricsEmissionITCase— MinIO via Testcontainers; real GET/HEAD/LIST round trips, asserting the counters/histograms are readable back through a realMetricRegistry(MetricListener). Auto-skips without Docker.Manual end-to-end against real AWS S3
I ran a standalone cluster built from this branch with
s3.metrics.enabled: trueand the SLF4J reporter, and submitted a large-state streaming job checkpointing tos3://<bucket>/checkpoints(HashMap backend, filesystem checkpoint storage, 10 s interval). The native plugin loaded (Plugin loader ... s3-fs-native), built its client via the SDK default credential chain, and wrote real checkpoint objects to S3. The reporter then showed the metrics on both the TaskManager (data-plane writes) and the JobManager (checkpoint coordination / multipart):Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
this change introduces the feature, followup documentation is up next.
Was generative AI tooling used to co-author this PR?