feat: update iceberg writer and add more metrics #13893
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #13893 +/- ##
==========================================
+ Coverage 67.99% 68.01% +0.02%
==========================================
Files 1536 1540 +4
Lines 265403 265453 +50
==========================================
+ Hits 180462 180556 +94
+ Misses 84941 84897 -44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| .deref() | ||
| .clone(), |
There was a problem hiding this comment.
The PrometheusWriterBuilder is defined in icelake, which means we are not able to choose the metric type we expect. If it's using the raw type of metric, there is no way to deal with the issue, even if we migrate to another prometheus library. The only way seems to be forking the current library to embed the current label guard logic in the internal implementation of current library and add a crate patch to the cargo.toml.
@ZENOTME @liurenjie1024 It is possible to let PrometheusWriterBuilder define some metric trait and pass a box dyn trait to it so that it can support customized metric report logic?
There was a problem hiding this comment.
Or we should the promtheus writer builder in risinwave?
There was a problem hiding this comment.
It is possible to let PrometheusWriterBuilder define some metric trait and pass a box dyn trait to it so that it can support customized metric report logic?
Yes, it's possible.
By deref and clone here, the ref count for label cannot work correctly.
Actually the semantic here is more like to extract the ref of metrics. The ref count don't need to inc if we can express the ref semantic which means the life time of the ref metrics is subrange to the metrics. So another solution maybe :
- label metrics have an interface to expose the ref of raw metrics.
- PrometheusWriterBuilder can take a ref of raw metrics. (I'm not sure whether it can
Or we should the promtheus writer builder in risinwave?
Yes. We can also implement our own Prometheus writer builder.
There was a problem hiding this comment.
This appears to be unsound. cc @wenym1 Is there a way to prevent this kind of leaking?
It seems hard to prevent because Deref makes it convenient to use the inner type. Except we expose the inner type op (add, set) manually.
impl<T, const N: usize> Deref for LabelGuardedMetric<T, N> {
type Target = T;
fn deref(&self) -> &Self::Target {
&self.inner
}
}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 4577 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 2009 | 5 | 2563 | 0 |
Click to see the invalid file list
- src/connector/src/sink/iceberg/prometheus/base_file_metrics_writer.rs
- src/connector/src/sink/iceberg/prometheus/mod.rs
- src/connector/src/sink/iceberg/prometheus/partition_metrics_writer.rs
- src/connector/src/sink/iceberg/prometheus/position_delete_metrics_writer.rs
- src/connector/src/sink/iceberg/prometheus/write_metrics_writer.rs
src/connector/src/sink/iceberg/prometheus/base_file_metrics_writer.rs
Outdated
Show resolved
Hide resolved
src/connector/src/sink/iceberg/prometheus/partition_metrics_writer.rs
Outdated
Show resolved
Hide resolved
src/connector/src/sink/iceberg/prometheus/position_delete_metrics_writer.rs
Outdated
Show resolved
Hide resolved
src/connector/src/sink/iceberg/prometheus/write_metrics_writer.rs
Outdated
Show resolved
Hide resolved
|
|
||
| async fn write(&mut self, record: RecordBatch) -> Result<()> { | ||
| self.write_qps.inc(); | ||
| let _ = self.write_latency.start_timer(); |
There was a problem hiding this comment.
Variable named with _ will be dropped immediately, in which case we will fail to measure the time. We should name it as _timer to avoid this.
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| pub mod base_file_metrics_writer; |
There was a problem hiding this comment.
In our naming convention we usually name it as monitored_xxx_writer;
| .iceberg_position_delete_cache_num | ||
| .clone(), | ||
| ); | ||
| let euality_delete_builder = |
| [ | ||
| panels.target( | ||
| f"{metric('iceberg_partition_num')}", | ||
| "{{executor_id}} @ {{sink_id}}", |
There was a problem hiding this comment.
what is the exec id in RW? I suggest using actor id instead
There was a problem hiding this comment.
what is the exec id in RW? I suggest using actor id instead
I think it means
risingwave/src/stream/src/executor/mod.rs
Line 172 in c8d351b
Why not use, cc @wenym1
There was a problem hiding this comment.
Just to be compatible with the code before I introduce this refactor, because before I introduce this refactor, the sink metric was using the executor id to form a identity string as the label.
I think we can change to directly using the actor id or executor id, but we should remember to change the grafana dashboard at the meantime.
There was a problem hiding this comment.
Let's refactor them all in a separate PR.
ZENOTME
force-pushed
the
zj/iceberg
branch
2 times, most recently
from
4d92681 to
360cbdc
Compare
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| pub mod monitor_base_file_writer; |
There was a problem hiding this comment.
Besides, the struct defined in these mods can be renamed to MonitoredXxxWriter as well.
| } | ||
|
|
||
| pub struct FanoutPartitionedWriterWithMetrics<B: IcebergWriterBuilder> | ||
| where |
There was a problem hiding this comment.
Is this where necessary?
I think there should already be type R: IcebergWriter trait bound on the IcebergWriterBuilder trait definition. If not, I think we should add the trait bound in the trait definition instead of here in the impl.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Close #12352
The icelake introduce new writer interface, this PR update to use them and add more metrics:
Checklist
- [ ] I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).- [ ] My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future)../risedev check(or alias,./risedev c)- [ ] My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)- [ ] My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)Documentation
- [ ] My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.