Integrate GMP fork packages and logic (3.13)#329
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates Google Cloud Managed Service for Prometheus (GMP) support into the Prometheus fork. Key additions include a GCM exporter, a Kubernetes-based leader election lease, a Kubernetes secret provider, and a proxy (prw2gcm) to convert Remote Write 2.x to GCM v3 TimeSeries. It also introduces GMP-specific configuration options and CLI flags. The review feedback is highly constructive and should be addressed: it highlights a security risk regarding missing TLS options when forwarding bearer tokens, suggests tightening the Authorization header validation to strictly check for the 'Bearer' prefix, recommends removing a fragile sed command in the Makefile, and points out minor code cleanups like removing an obsolete TODO and an uncertain import comment.
|
Thanks! Do you mind removing unnecessary checks and docs as we did in the past (clarity, clean CI): https://github.com/GoogleCloudPlatform/prometheus-engine/pull/1712/changes#r3571934103 |
7d4a7b9 to
4ac9c11
Compare
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
Thanks! It looks mostly ok. Biggest comments are on google/ dir, UTF-8 feature , agent mode and Dockerfile
Re export: Let's make sure we are very strict on google/ dir changes to avoid surprises. Those should be 1:1 copied from the old version and only changed to make it build, without unrelated changes. Here is the preview of what changed: #330
Let's also manually check UI if it works with basic ops on a local container.
Thanks!
| @@ -858,8 +871,12 @@ func (*DB) ExemplarQuerier(context.Context) (storage.ExemplarQuerier, error) { | |||
| } | |||
|
|
|||
| // Appender implements storage.Storage. | |||
| func (db *DB) Appender(context.Context) storage.Appender { | |||
| return db.appenderPool.Get().(storage.Appender) | |||
| func (db *DB) Appender(ctx context.Context) storage.Appender { | |||
There was a problem hiding this comment.
This is unused method. We can keep this change for consistency, but let's ensure we change AppenderV2 as well in https://github.com/prometheus/prometheus/blob/main/tsdb/agent/db_append_v2.go
Can we add issue on this fork to add GCM test for agent mode?
| } | ||
| } | ||
| return h.appenderV2() | ||
| app := h.appenderV2() |
There was a problem hiding this comment.
nit: Is there a reason above we do appender(metadataFunc) and here we don't and set manually instead? I'd stay consistent
| @@ -0,0 +1,134 @@ | |||
| # This dockerfile is multi target. Use DOCKER_BUILDKIT=1 when building and reference the target: | |||
There was a problem hiding this comment.
Generated summary for a diff, it looks mostly ok. The vendor copy is interesting, sounds like we missed that in the old Dockerfile? 🙈
The NPM changes are also unique, but they seems to be fine. Let's ensure UI works after we build docker container.
Dockerfile.google Diff: release-2.53.5-gmp vs bernot-dev/release-3.13.0-gmp
Comparing branches:
- Base (
release-2.53.5-gmp):Dockerfile.google - Target (
bernot-dev/release-3.13.0-gmp):Dockerfile.google
Executive Summary of Changes
| Category | Key Changes | Rationale / Upstream v3 Impact |
|---|---|---|
Node / Package Manager (pnpm) |
Transitioned from npm / make ui-install to pnpm install with Corepack enabled. Added UV_USE_IO_URING=0 and UV_THREADPOOL_SIZE=1. |
Upstream Prometheus v3 UI (mantine-ui + react-app) uses pnpm. UV environment flags prevent libuv threadpool deadlocks under QEMU multi-arch emulation. |
Go Workspace (GOWORK) & Vendoring |
Added ENV GOWORK=off to govendor and buildbase. Explicitly copied /workspace/vendor into buildbase. |
Ensures Go 1.22+ modules do not look outside the repository or fail if local go.work files exist during vendored builds. |
| BuildKit Multi-Arch Args | Added explicit ARG TARGETARCH, ARG TARGETOS, ARG BUILDARCH before and inside buildbase. |
Fixes variable scoping in multi-stage BuildKit builds so cross-compilation condition checks work correctly across stages. |
UI Asset Stages (assets, vendor) |
assets now inherits from nodevendor (instead of nodebase). Added mantine-ui node modules copy. Removed make npm_licenses. |
Supports Prometheus v3 Mantine UI assets build pipeline. Replaces rigid make errors with existence checks if static assets are already compiled. |
| Removal of Console Templates | Removed console_libraries/ and consoles/ copies, symlinks, and --web.console.libraries / --web.console.templates flags from CMD. |
Upstream Prometheus v3 deprecated and cleaned up classic legacy console templates from default runtime configurations. |
Exact Git Diff (Dockerfile.google)
diff --git a/Dockerfile.google b/Dockerfile.google
index 74ce91228f..60db1deeb4 100644
--- a/Dockerfile.google
+++ b/Dockerfile.google
@@ -23,10 +23,15 @@ USER root
# from the gobase, rest from apt.
COPY --from=gobase /usr/local/go /usr/local/
ENV PATH="/usr/local/go/bin:${PATH}"
+ENV UV_USE_IO_URING=0
+ENV UV_THREADPOOL_SIZE=1
RUN apt-get update
RUN apt-get -y install bzip2 make git
+RUN corepack enable pnpm || npm install -g pnpm
+RUN pnpm config set child-concurrency 1
# Verify early if we have all we need.
RUN npm version
+RUN pnpm --version
RUN make -v
RUN git --version
RUN bzip2 --version
@@ -35,6 +40,7 @@ RUN go version
# --target=vendor
FROM gobase AS govendor
COPY . ./
+ENV GOWORK=off
RUN go mod vendor
FROM nodebase AS nodevendor
@@ -48,30 +54,37 @@ COPY . ./
# different Prometheus versions.
# TODO(bwplotka): Consider moving those deps in upstream to non-dev lists.
ENV NODE_ENV="development"
-RUN make ui-install
+RUN (cd web/ui && pnpm install) || [ -d web/ui/node_modules/vite ]
+RUN (cd web/ui/react-app && pnpm install) || [ -d web/ui/react-app/node_modules/react ]
FROM scratch AS vendor
COPY --from=govendor /workspace/vendor vendor
COPY --from=nodevendor /workspace/web/ui/node_modules web/ui/node_modules
+COPY --from=nodevendor /workspace/web/ui/mantine-ui/node_modules web/ui/mantine-ui/node_modules
COPY --from=nodevendor /workspace/web/ui/module/codemirror-promql/node_modules web/ui/module/codemirror-promql/node_modules
COPY --from=nodevendor /workspace/web/ui/react-app/node_modules web/ui/react-app/node_modules
# --target=app
# Compile the UI assets.
-FROM nodebase AS assets
-COPY . ./
-# Only build the UI but don't run ui-install; deps should be installed in separate step (--target=vendor).
-RUN make ui-build
+FROM nodevendor AS assets
+RUN (make ui-build) || ( [ -d web/ui/static/react-app ] && [ -d web/ui/static/mantine-ui ] )
RUN scripts/compress_assets.sh
-RUN make npm_licenses
# Build the actual Go binary.
+ARG TARGETARCH
+ARG TARGETOS
+ARG BUILDARCH
FROM gobase AS buildbase
+ARG TARGETARCH
+ARG TARGETOS
+ARG BUILDARCH
COPY --from=assets /workspace ./
+COPY --from=govendor /workspace/vendor vendor
ENV GOEXPERIMENT=boringcrypto
ENV CGO_ENABLED=1
ENV GOFIPS140=off
ENV GOTOOLCHAIN=local
+ENV GOWORK=off
ENV GOARCH=${TARGETARCH}
ENV GOOS=${TARGETOS}
RUN if [ "${TARGETARCH}" = "arm64" ] && [ "${BUILDARCH}" != "arm64" ]; then \
@@ -101,9 +114,6 @@ RUN if [ "${TARGETARCH}" = "arm64" ] && [ "${BUILDARCH}" != "arm64" ]; then \
FROM ${IMAGE_BASE_DEBUG} AS appbase
COPY documentation/examples/prometheus.yml /etc/prometheus/prometheus.yml
-COPY console_libraries/ /usr/share/prometheus/console_libraries/
-COPY consoles/ /usr/share/prometheus/consoles/
-RUN ["/busybox/sh", "-c", "ln -s /usr/share/prometheus/console_libraries /usr/share/prometheus/consoles/ /etc/prometheus/"]
RUN ["/busybox/sh", "-c", "mkdir -p /prometheus"]
FROM ${IMAGE_BASE} AS app
@@ -113,16 +123,12 @@ COPY --from=buildbase /workspace/promtool /bin/promtool
COPY --from=buildbase /workspace/prw2gcm /bin/prw2gcm
COPY --from=appbase --chown=nobody:nobody /etc/prometheus /etc/prometheus
COPY --from=appbase --chown=nobody:nobody /prometheus /prometheus
-COPY --from=appbase /usr/share/prometheus /usr/share/prometheus
COPY LICENSE /LICENSE
COPY NOTICE /NOTICE
-COPY --from=assets /workspace/npm_licenses.tar.bz2 /npm_licenses.tar.bz2
USER nobody
EXPOSE 9090
VOLUME [ "/prometheus" ]
ENTRYPOINT [ "/bin/prometheus" ]
CMD [ "--config.file=/etc/prometheus/prometheus.yml", \
- "--storage.tsdb.path=/prometheus", \
- "--web.console.libraries=/usr/share/prometheus/console_libraries", \
- "--web.console.templates=/usr/share/prometheus/consoles" ]
+ "--storage.tsdb.path=/prometheus" ]There was a problem hiding this comment.
Did you manage to check UI?
c65f735 to
f1e1914
Compare
f1e1914 to
22269e0
Compare
| // changed and applied to the exporter, potentially recreating the metric client. | ||
| // NOTE: Runtime configuration will not override disable/disableAuth options from flags. | ||
| func (e *Exporter) ApplyConfig(cfg *config.Config) (err error) { | ||
| if e.opts.Disable { |
There was a problem hiding this comment.
Why fixing now though in this big PR?
There were some semantics for credentials that we need to be mindful that's why I was curious if you are sure it this does not have side effects, sounds like you are
| // It returns when a series is consumed which completes a full distribution. | ||
| // Once all series for a single distribution have been observed, it returns it. | ||
| // It returns the reset timestamp along with the distribution and the remaining samples. | ||
| func (b *sampleBuilder) buildDistribution( |
| return watcher, nil | ||
| } | ||
|
|
||
| func (w *secretWatcher) update(logger log.Logger, e watch.Event, config *KubernetesSecretConfig) { |
There was a problem hiding this comment.
As per https://github.com/GoogleCloudPlatform/prometheus/pull/330/changes#r3587278927 let's add test for the new change secret.Name == config.Name
| @@ -858,8 +871,12 @@ func (*DB) ExemplarQuerier(context.Context) (storage.ExemplarQuerier, error) { | |||
| } | |||
|
|
|||
| // Appender implements storage.Storage. | |||
| func (db *DB) Appender(context.Context) storage.Appender { | |||
| return db.appenderPool.Get().(storage.Appender) | |||
| func (db *DB) Appender(ctx context.Context) storage.Appender { | |||
| return db.appenderPool.Get().(storage.Appender) | ||
| func (db *DB) Appender(ctx context.Context) storage.Appender { | ||
| a := db.appenderPool.Get().(*appender) | ||
| // Leave metadata getter as nil if it's not contained in the context. |
There was a problem hiding this comment.
Is this comment useful? (also not present on v2)
| } | ||
| } | ||
| return h.appenderV2() | ||
| app := h.appenderV2() |
| @@ -0,0 +1,134 @@ | |||
| # This dockerfile is multi target. Use DOCKER_BUILDKIT=1 when building and reference the target: | |||
There was a problem hiding this comment.
Did you manage to check UI?
|
Regenerated #330 We are mostly missing interleaving histogram fix and some tests to verify new watcher fix. |
bernot-dev
changed the title
… prw2gcm, promtest) Signed-off-by: Adam Bernot <bernot@google.com>
Signed-off-by: Adam Bernot <bernot@google.com>
Signed-off-by: Adam Bernot <bernot@google.com>
22269e0 to
73c19ac
Compare
3 participants
Commit Breakdown: