Skip to content
Draft
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@
"Nbits",
"Nocie",
"ONNX",
"openai",
"Oneof",
"Outdegree",
"Outf",
Expand Down Expand Up @@ -1196,13 +1197,15 @@
"brandguidelines",
"clusterrolebinding",
"conflint",
"conninfo",
"contributorsrc",
"conv",
"darkgray",
"getstarted",
"gitfiles",
"gongt",
"helmignore",
"hono",
"kosarak",
"kustomization",
"lycorp",
Expand All @@ -1211,6 +1214,7 @@
"nytimes",
"priorityclass",
"promtail",
"argparse",
"serviceaccount",
"sidecard",
"testdata",
Expand Down
38 changes: 38 additions & 0 deletions .gitfiles
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,9 @@ apis/grpc/v1/agent/sidecar/sidecar.pb.go
apis/grpc/v1/agent/sidecar/sidecar_vtproto.pb.go
apis/grpc/v1/discoverer/discoverer.pb.go
apis/grpc/v1/discoverer/discoverer_vtproto.pb.go
apis/grpc/v1/embedder/embedder.pb.go
apis/grpc/v1/embedder/embedder.pb.json.go
apis/grpc/v1/embedder/embedder_vtproto.pb.go
apis/grpc/v1/filter/egress/egress_filter.pb.go
apis/grpc/v1/filter/egress/egress_filter_vtproto.pb.go
apis/grpc/v1/filter/ingress/ingress_filter.pb.go
Expand Down Expand Up @@ -241,6 +244,7 @@ apis/grpc/v1/vald/vald.go
apis/proto/v1/agent/core/agent.proto
apis/proto/v1/agent/sidecar/sidecar.proto
apis/proto/v1/discoverer/discoverer.proto
apis/proto/v1/embedder/embedder.proto
Comment thread
Matts966 marked this conversation as resolved.
Comment thread
Matts966 marked this conversation as resolved.
apis/proto/v1/filter/egress/egress_filter.proto
apis/proto/v1/filter/ingress/ingress_filter.proto
apis/proto/v1/meta/meta.proto
Expand All @@ -265,6 +269,7 @@ apis/proto/v1/vald/upsert.proto
apis/swagger/v1/agent/core/agent.swagger.json
apis/swagger/v1/agent/sidecar/sidecar.swagger.json
apis/swagger/v1/discoverer/discoverer.swagger.json
apis/swagger/v1/embedder/embedder.swagger.json
apis/swagger/v1/filter/egress/egress_filter.swagger.json
apis/swagger/v1/filter/ingress/ingress_filter.swagger.json
apis/swagger/v1/meta/meta.swagger.json
Expand Down Expand Up @@ -606,6 +611,9 @@ cmd/tools/cli/benchmark/core/main_test.go
cmd/tools/cli/vdctl/doc.go
cmd/tools/cli/vdctl/main.go
cmd/tools/cli/vdctl/main_test.go
cmd/tools/embedder/doc.go
cmd/tools/embedder/main.go

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Several new embedder implementation files ship without matching unit tests.

From this manifest, cmd/tools/embedder/main.go, pkg/tools/embedder/handler/grpc/handler.go, pkg/tools/embedder/handler/grpc/option.go, the pkg/tools/embedder/service/*.go files, and the pkg/tools/embedder/usecase/*.go files do not have corresponding *_test.go entries. That leaves most of the new component’s core paths unverified in this PR.

As per coding guidelines: "Each implementation file must come with its unit test file with *_test.go postfix".

Also applies to: 2287-2305

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.gitfiles at line 615, Add unit test files for every new implementation file
listed: create *_test.go files for cmd/tools/embedder/main.go,
pkg/tools/embedder/handler/grpc/handler.go,
pkg/tools/embedder/handler/grpc/option.go and for each file under
pkg/tools/embedder/service/ and pkg/tools/embedder/usecase/. For each test file,
target the exported functions and methods (eg. the Serve/Handle methods in
grpc/handler.go, option parsing in option.go, and each public service/usecase
function), use table-driven tests for normal and error cases, and inject/mocк
dependencies (interfaces) to isolate logic; place tests in the same package or
package_name_test as appropriate and name files with the _test.go suffix so they
run under go test. Ensure coverage includes success, failure, and edge paths and
that any gRPC handlers are exercised via grpc/test utilities or mocked servers.

cmd/tools/embedder/sample.yaml
codecov.yaml
design/Vald Architecture Assets.drawio
design/Vald Architecture Dataflow.drawio
Expand Down Expand Up @@ -836,6 +844,7 @@ hack/tools/deadlink/main.go
hack/tools/kvsdb/main.go
hack/tools/metrics/main.go
hack/tools/metrics/main_test.go
hack/tools/openai-embedding-mock/main.go
internal/backoff/backoff.go
internal/backoff/backoff_test.go
internal/backoff/context.go
Expand Down Expand Up @@ -2271,6 +2280,29 @@ pkg/tools/benchmark/operator/service/option_test.go
pkg/tools/benchmark/operator/usecase/benchmarkd.go
pkg/tools/benchmark/operator/usecase/benchmarkd_test.go
pkg/tools/benchmark/operator/usecase/doc.go
pkg/tools/embedder/README.md
pkg/tools/embedder/config/config.go
pkg/tools/embedder/config/config_test.go
pkg/tools/embedder/handler/doc.go
pkg/tools/embedder/handler/grpc/handler.go
pkg/tools/embedder/handler/grpc/option.go
pkg/tools/embedder/handler/rest/handler.go
pkg/tools/embedder/handler/rest/handler_test.go
pkg/tools/embedder/handler/rest/option.go
pkg/tools/embedder/handler/rest/option_test.go
pkg/tools/embedder/router/option.go
pkg/tools/embedder/router/option_test.go
pkg/tools/embedder/router/router.go
pkg/tools/embedder/router/router_test.go

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[file name cspell] reported by reviewdog 🐶
Unknown word (devserver) Suggestions: [deserver, deserve, deserter, deserved, deserves]

pkg/tools/embedder/service/doc.go
pkg/tools/embedder/service/embedder.go
pkg/tools/embedder/service/llm.go
pkg/tools/embedder/service/llm_option.go

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[file name cspell] reported by reviewdog 🐶
Unknown word (esbuild) Suggestions: [ebuild, tsbuild, TSBuild, build, rebuild]

pkg/tools/embedder/service/meta.go
pkg/tools/embedder/service/option.go
pkg/tools/embedder/usecase/agentd.go
pkg/tools/embedder/usecase/doc.go
pkg/tools/embedder/usecase/embedder.go
renovate.json
rust/Cargo.lock
rust/Cargo.toml
Expand Down Expand Up @@ -2328,6 +2360,10 @@ rust/libs/proto/src/core/v1/mod.rs
rust/libs/proto/src/discoverer/mod.rs
rust/libs/proto/src/discoverer/v1/discoverer.v1.tonic.rs
rust/libs/proto/src/discoverer/v1/mod.rs
rust/libs/proto/src/embedder.v1.tonic.rs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[file name cspell] reported by reviewdog 🐶
Unknown word (sveltekit) Suggestions: [sveltest, sleekit, svelte, svelter, sveltely]

rust/libs/proto/src/embedder/v1/embedder.v1.rs
rust/libs/proto/src/embedder/v1/embedder.v1.serde.rs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[file name cspell] reported by reviewdog 🐶
Unknown word (tanstack) Suggestions: [daostack, jamstack, DAOstack, attack, tanta]

rust/libs/proto/src/embedder/v1/embedder.v1.tonic.rs
rust/libs/proto/src/filter/egress/mod.rs
rust/libs/proto/src/filter/egress/v1/filter.egress.v1.tonic.rs
rust/libs/proto/src/filter/egress/v1/mod.rs
Expand Down Expand Up @@ -2394,6 +2430,7 @@ tests/e2e/pkg/agent/core/ngt/service/ngt_e2s_test.go
tests/e2e/sidecar/sidecar_test.go
tests/performance/max_vector_dim_test.go
tests/v2/e2e/assets/agent_crud.yaml
tests/v2/e2e/assets/embedder.yaml
tests/v2/e2e/assets/index_correction.yaml
tests/v2/e2e/assets/multi_crud.yaml
tests/v2/e2e/assets/readreplica.yaml
Expand All @@ -2405,6 +2442,7 @@ tests/v2/e2e/config/enums.go
tests/v2/e2e/crud/agent_test.go
tests/v2/e2e/crud/crud_test.go
tests/v2/e2e/crud/dataset_test.go
tests/v2/e2e/crud/embedder_test.go
tests/v2/e2e/crud/grpc_test.go
tests/v2/e2e/crud/index_test.go
tests/v2/e2e/crud/kubernetes_test.go
Expand Down
Empty file modified .github/ISSUE_TEMPLATE/bug_report.md
100755 → 100644
Empty file.
Empty file modified .github/ISSUE_TEMPLATE/feature_request.md
100755 → 100644
Empty file.
Empty file modified .github/PULL_REQUEST_TEMPLATE.md
100755 → 100644
Empty file.
108 changes: 108 additions & 0 deletions .github/workflows/e2e.v2.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ jobs:
{ scenario: "stream_crud", deployment: "helm-chart", cluster: "k3d", environment: "null" },
{ scenario: "unary_crud", deployment: "helm-chart", cluster: "k3d", environment: "null" },
{ scenario: "multi_crud", deployment: "helm-chart", cluster: "k3d", environment: "null" },
{ scenario: "embedder", deployment: "helm-chart", cluster: "k3d", environment: "null" },
{ scenario: "rollout", deployment: "helm-chart", cluster: "k3d", environment: "null" },
{ scenario: "stream_crud", deployment: "helm-operator", cluster: "k3d", environment: "null" },
{ scenario: "index_correction", deployment: "helm-chart", cluster: "k3d", environment: "management" },
Expand Down Expand Up @@ -225,6 +226,105 @@ jobs:
with:
helm_extra_options: "--set networkChaos.${{ matrix.environment }}.enabled=true"

- name: Start Mock OpenAI compatible embedding API
if: ${{ matrix.scenario == 'embedder' }}
shell: bash
run: |
nohup go run ./hack/tools/openai-embedding-mock >/tmp/mock-openai.log 2>&1 &
for i in $(seq 1 30); do
if curl -fsS http://127.0.0.1:18000/healthz >/dev/null; then
break
fi
sleep 1
done
curl -fsS http://127.0.0.1:18000/healthz >/dev/null

- name: Start Embedder for E2E smoke scenario
if: ${{ matrix.scenario == 'embedder' }}
shell: bash
run: |
kubectl port-forward -n default svc/vald-lb-gateway 18081:8081 >/tmp/vald-port-forward.log 2>&1 &
kubectl port-forward -n default svc/vald-meta-gateway 18082:8081 >/tmp/meta-port-forward.log 2>&1 &
cat > /tmp/embedder-e2e.yaml <<'YAML'
version: v0.0.0
time_zone: UTC
logging:
format: raw
level: info
logger: glg
server_config:
servers:
- name: grpc
host: 0.0.0.0
port: 28081
grpc:
bidirectional_stream_concurrency: 20
mode: GRPC
probe_wait_time: 1s
restart: true
health_check_servers:
- name: liveness
host: 0.0.0.0
port: 3300
http:
shutdown_duration: 5s
probe_wait_time: 1s
- name: readiness
host: 0.0.0.0
port: 3301
http:
shutdown_duration: 0s
probe_wait_time: 1s
startup_strategy:
- liveness
- grpc
- readiness
full_shutdown_duration: 30s
observability:
enabled: false
client:
addrs:
- 127.0.0.1:18081
health_check_duration: 1s
connection_pool:
enable_dns_resolver: true
enable_rebalance: true
old_conn_close_duration: 3s
rebalance_duration: 30m
size: 1
call_option:
wait_for_ready: true
dial_option:
insecure: true
min_connection_timeout: 20s
meta:
host: 127.0.0.1
port: 18082
client:
addrs:
- 127.0.0.1:18082
health_check_duration: 1s
connection_pool:
enable_dns_resolver: true
enable_rebalance: true
old_conn_close_duration: 3s
rebalance_duration: 30m
size: 1
call_option:
wait_for_ready: true
dial_option:
insecure: true
min_connection_timeout: 20s
llm:
provider: openai
openai:
token: test-token
model: text-embedding-3-small
base_url: http://127.0.0.1:18000/v1
YAML
nohup go run ./cmd/tools/embedder -c /tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 &
sleep 10

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace hardcoded sleep 10 with a readiness poll on the embedder's health endpoint.

The mock-OpenAI step correctly polls /healthz before proceeding, but the embedder start step uses a fixed 10 s sleep. Go compilation under a cold cache or a slow cluster can easily exceed this, causing the subsequent make e2e/v2 run to hit a "connection refused" error on port 28081. The embedder config already exposes a readiness endpoint at port 3301.

🛠️ Proposed fix
-          nohup go run ./cmd/tools/embedder -c /tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 &
-          sleep 10
+          nohup go run ./cmd/tools/embedder -c /tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 &
+          for i in $(seq 1 60); do
+            if curl -fsS http://127.0.0.1:3301/readiness >/dev/null 2>&1; then
+              break
+            fi
+            sleep 1
+          done
+          curl -fsS http://127.0.0.1:3301/readiness >/dev/null
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
nohup go run ./cmd/tools/embedder -c /tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 &
sleep 10
nohup go run ./cmd/tools/embedder -c /tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 &
for i in $(seq 1 60); do
if curl -fsS http://127.0.0.1:3301/readiness >/dev/null 2>&1; then
break
fi
sleep 1
done
curl -fsS http://127.0.0.1:3301/readiness >/dev/null
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/e2e.v2.yaml around lines 325 - 326, The fixed sleep should
be replaced with a readiness poll against the embedder's health endpoint instead
of the hardcoded "sleep 10": after launching the embedder with "nohup go run
./cmd/tools/embedder -c /tmp/embedder-e2e.yaml >/tmp/embedder-e2e.log 2>&1 &",
implement a loop that repeatedly curls http://127.0.0.1:3301/healthz (or
/healthz) with a short backoff until it returns success or a reasonable timeout
is reached; if the timeout expires, print /tmp/embedder-e2e.log and fail the
step so downstream "make e2e/v2" doesn’t hit connection refused on port 28081.
Ensure the probe uses --fail/exit codes so the workflow step can detect
readiness reliably.


- name: Deploy profefe
if: ${{ matrix.environment == 'profile' }}
shell: bash
Expand Down Expand Up @@ -263,6 +363,14 @@ jobs:
kubectl get pods
kubectl describe pods
kubectl describe pvc
if [[ -f /tmp/mock-openai.log ]]; then
echo "--- /tmp/mock-openai.log ---"
cat /tmp/mock-openai.log
fi
if [[ -f /tmp/embedder-e2e.log ]]; then
echo "--- /tmp/embedder-e2e.log ---"
cat /tmp/embedder-e2e.log
fi

slack-notification:
timeout-minutes: 30
Expand Down
Loading
Loading