Skip to content
Draft
Show file tree
Hide file tree
Changes from all 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.
115 changes: 115 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,112 @@ 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: 30s
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 is optional: comment out when vald-meta-gateway is not deployed
# meta:
# host: 127.0.0.1
# port: 18082
# client:
# addrs:
# - 127.0.0.1:18082
# health_check_duration: 30s
# 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 &
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

- name: Deploy profefe
if: ${{ matrix.environment == 'profile' }}
shell: bash
Expand Down Expand Up @@ -263,6 +370,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