GCP: Add CloudMonitoringMetricsReporter to export Iceberg metrics to Cloud Monitoring#16107
GCP: Add CloudMonitoringMetricsReporter to export Iceberg metrics to Cloud Monitoring#16107talatuyarer wants to merge 1 commit into
Conversation
talatuyarer
force-pushed
the
gcp-cloud-monitoring
branch
4 times, most recently
from
60316ce to
fc34a15
Compare
stevenzwu
changed the title
| implementation "com.google.cloud:google-cloud-bigquery" | ||
| implementation "com.google.cloud:google-cloud-core" | ||
| implementation "com.google.cloud:google-cloud-kms" | ||
| implementation "com.google.cloud:google-cloud-monitoring" |
There was a problem hiding this comment.
we should be very careful with dependency change for runtime bundle jar. was this PR created after all the dependency/license fixes. at least, rebase with the latest main branch. CI should catch dependency/license problems nowadays.
There was a problem hiding this comment.
Yes I created this PR Last Friday. How can I verify that ? There is also license check too
https://github.com/apache/iceberg/actions/runs/25008117162/job/73236359466?pr=16107
There was a problem hiding this comment.
+1 #16103 + #16081 should enforce any runtime dep changes in CI, via [check-runtime-deps](https://github.com/apache/iceberg/actions/runs/25399058172/job/74493503774?pr=16107#logs) task
Theres also #16182 which is updating the LICENSE for GCP bundle.
Lets wait for all 3 PRs to land so we have a good baseline, and then we can look at adding new deps
There was a problem hiding this comment.
i would expect the runtime-deps.txt file to be modifed based on this addition...
There was a problem hiding this comment.
I didn't see the GitHub action for licensing checks run.
@talatuyarer - can you rebase off of main to make sure that you've got Russell's new dependency checks?
There was a problem hiding this comment.
looks like it was already pulled transitively and already part of runtime-deps.txt, hence no diff here
iceberg/gcp-bundle/runtime-deps.txt
Line 44 in b7ef9f1
👍
talatuyarer
force-pushed
the
gcp-cloud-monitoring
branch
from
c94ffc9 to
3a062b4
Compare
| * GCS project ID. | ||
| */ | ||
| public static final String GCP_MONITORING_PROJECT_ID = "gcp.monitoring.project-id"; | ||
|
|
||
| /** Controls the metric prefix used for Google Cloud Monitoring metrics. */ | ||
| public static final String GCP_MONITORING_METRIC_PREFIX = "gcp.monitoring.metric-prefix"; | ||
|
|
||
| /** Default metric prefix for Google Cloud Monitoring metrics. */ | ||
| public static final String GCP_MONITORING_METRIC_PREFIX_DEFAULT = "custom.googleapis.com/iceberg"; | ||
|
|
There was a problem hiding this comment.
nit: gcs vs gcp. i see we use gcs. prefix in all the other places. If we are starting to use gcp., we should document it
There was a problem hiding this comment.
I have a slight bias towards using GCP here because this isn't storage related. Monitoring is a Cloud product, but not a storage product.
| implementation "com.google.cloud:google-cloud-bigquery" | ||
| implementation "com.google.cloud:google-cloud-core" | ||
| implementation "com.google.cloud:google-cloud-kms" | ||
| implementation "com.google.cloud:google-cloud-monitoring" |
There was a problem hiding this comment.
looks like it was already pulled transitively and already part of runtime-deps.txt, hence no diff here
iceberg/gcp-bundle/runtime-deps.txt
Line 44 in b7ef9f1
👍
talatuyarer
force-pushed
the
gcp-cloud-monitoring
branch
2 times, most recently
from
73cb399 to
8a17667
Compare
| import org.awaitility.Awaitility; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| /** |
There was a problem hiding this comment.
This isn't being run in CI, correct? Do you have some test logs you can show to help us understand that these tests pass?
talatuyarer
force-pushed
the
gcp-cloud-monitoring
branch
2 times, most recently
from
8a17667 to
671db0b
Compare
4 participants
This PR adds a new MetricsReporter implementation to the iceberg-gcp module that sends Iceberg scan and commit metrics to Google Cloud Monitoring.