Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Python SLO workload runner under tests/slo/ to use environment-variable-driven configuration (workload selection, YDB connection, duration, OTLP exporter settings) instead of CLI subcommands, and updates metrics and CI workflows accordingly.
Changes:
- Replace subcommand-based orchestration with a single “run all” flow selected via
WORKLOAD_NAME/ env vars. - Rework metrics initialization to use standard OpenTelemetry env vars and switch latency quantiles to HDR histogram-based calculation.
- Update SLO GitHub workflows and Docker image interface to match the new env-var-based runner.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/slo/src/runners/topic_runner.py | Update metrics factory call to new signature. |
| tests/slo/src/runners/table_runner.py | Update metrics factory call to new signature. |
| tests/slo/src/root_runner.py | Replace subcommand runner with run_all() flow selected via WORKLOAD_NAME. |
| tests/slo/src/pyrightconfig.json | Add Pyright configuration for the SLO runner code. |
| tests/slo/src/options.py | Simplify CLI flags; inject connection/duration from environment variables. |
| tests/slo/src/jobs/table_jobs.py | Switch to WORKLOAD_NAME constant. |
| tests/slo/src/jobs/base.py | Always start metrics sender job (no CLI OTLP gating). |
| tests/slo/src/jobs/async_topic_jobs.py | Always start async metrics sender task (no CLI OTLP gating). |
| tests/slo/src/core/metrics.py | Move to standard OTel env vars and implement HDR-based latency quantiles. |
| tests/slo/src/main.py | Switch entrypoint to call run_all(). |
| tests/slo/requirements.txt | Replace quantile-estimator with hdrhistogram. |
| tests/slo/Dockerfile | Update documentation/comments and provide a default CMD for tuning flags. |
| pyproject.toml | Configure tool.ty extra paths for SLO sources. |
| .github/workflows/slo.yml | Refactor workload matrix and switch to ydb-slo-action v2 init/report. |
| .github/workflows/slo-report.yml | Switch report action reference to v2 and remove label-removal job. |
Comments suppressed due to low confidence (1)
.github/workflows/slo.yml:53
concurrency.groupreferencesmatrix.workload, but the matrix is now defined undermatrix.sdk(withname/command). This will make the workflow fail to evaluate at runtime. Update the concurrency group expression to use the new matrix key (e.g.,matrix.sdk.name).
concurrency:
group: slo-${{ github.ref }}-${{ matrix.workload }}
cancel-in-progress: true
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vgvoleg
requested changes
Apr 3, 2026
Member
vgvoleg
left a comment
vgvoleg
left a comment
There was a problem hiding this comment.
In this PR you've deleted async jobs, which is out of scope; you've reorganized arguments, which is out of scope; you've deleted an ability to run jobs without monitoring job; you've added configs for tools we are not using...
Please make PR as simple as it needed to solve your task.
vgvoleg
requested changes
Apr 22, 2026
polRk
force-pushed
the
slo-v2
branch
2 times, most recently
from
5f7b4b3 to
e9f1f7e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+270
to
+286
| def create_metrics(args) -> BaseMetrics: | ||
| """ | ||
| Factory used by SLO runners. | ||
|
|
||
| Metrics are enabled if either: | ||
| - OTLP_ENDPOINT env var is set, or | ||
| - `--otlp-endpoint` is provided (and non-empty) | ||
|
|
||
| If endpoint is empty, metrics are disabled (DummyMetrics). | ||
| Uses args.otlp_endpoint, args.workload_name, args.workload_ref from parsed CLI arguments. | ||
| If the endpoint is empty, returns a no-op DummyMetrics. | ||
| """ | ||
| endpoint = (environ.get("OTLP_ENDPOINT") or (otlp_endpoint or "")).strip() | ||
| endpoint = (args.otlp_endpoint or "").strip() | ||
|
|
||
| if not endpoint: | ||
| logger.info("Creating dummy metrics (metrics disabled)") | ||
| logger.info("OTLP endpoint not configured — metrics disabled") | ||
| return DummyMetrics() | ||
|
|
||
| logger.info("Creating OTLP metrics exporter to Prometheus: %s", endpoint) | ||
| environ.setdefault("OTEL_EXPORTER_OTLP_PROTOCOL", "http/protobuf") | ||
| environ["OTEL_EXPORTER_OTLP_ENDPOINT"] = endpoint | ||
|
|
||
| logger.info("Creating OTLP metrics exporter (endpoint: %s)", endpoint) |
Comment on lines
+61
to
+63
| driver.stop(timeout=args.shutdown_time) | ||
|
|
||
|
|
| default="http://localhost:9090/api/v1/otlp/v1/metrics", | ||
| type=str, | ||
| help="Full Prometheus OTLP metrics endpoint (e.g. http://ydb-prometheus:9090/api/v1/otlp/v1/metrics). Empty to disable.", | ||
| Every flag supports a fallback chain: CLI arg > environment variable > hardcoded default. |
| - if: always() | ||
| name: Upload logs | ||
| uses: actions/upload-artifact@v4 | ||
| workload_baseline_image: ydb-app-current |
Comment on lines
+19
to
+35
| FROM python:3.11-slim | ||
|
|
||
| ENV PYTHONDONTWRITEBYTECODE=1 \ | ||
| PYTHONUNBUFFERED=1 | ||
|
|
||
| WORKDIR /src | ||
| COPY . /src | ||
|
|
||
| # Install runtime deps into an isolated venv so we can copy it into the final stage. | ||
| RUN python -m venv /opt/venv \ | ||
| && /opt/venv/bin/python -m pip install --no-cache-dir --upgrade pip \ | ||
| && /opt/venv/bin/pip install --no-cache-dir . \ | ||
| && /opt/venv/bin/pip install --no-cache-dir -r tests/slo/requirements.txt | ||
| RUN apt-get update && apt-get install -y --no-install-recommends gcc libc6-dev && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| WORKDIR /src | ||
|
|
||
| FROM python:3.11-slim | ||
| # 1. YDB SDK | ||
| COPY setup.py pyproject.toml README.md requirements.txt ./ | ||
| COPY ydb/ ydb/ | ||
| RUN pip install --no-cache-dir . | ||
|
|
||
| ENV PYTHONDONTWRITEBYTECODE=1 \ | ||
| PYTHONUNBUFFERED=1 \ | ||
| PATH="/opt/venv/bin:${PATH}" | ||
| # 2. SLO deps | ||
| COPY tests/slo/requirements.txt tests/slo/requirements.txt | ||
| RUN pip install --no-cache-dir -r tests/slo/requirements.txt |
Comment on lines
+170
to
+185
| # Latency gauges (fed from HDR histograms via push()) | ||
| self._latency_p50 = self._meter.create_gauge( | ||
| name="sdk_operation_latency_p50_seconds", | ||
| unit="s", | ||
| description="P50 operation latency in seconds.", | ||
| ) | ||
| self._latency_p95 = self._meter.create_gauge( | ||
| name="sdk_operation_latency_p95_seconds", | ||
| unit="s", | ||
| description="P95 operation latency in seconds.", | ||
| ) | ||
| self._latency_p99 = self._meter.create_gauge( | ||
| name="sdk_operation_latency_p99_seconds", | ||
| unit="s", | ||
| description="P99 operation latency in seconds.", | ||
| ) |
vgvoleg
requested changes
Apr 27, 2026
polRk
force-pushed
the
slo-v2
branch
2 times, most recently
from
3ce18f6 to
2d4e027
Compare
🌋 SLO Test Results🟢 2 workload(s) tested — All thresholds passed
Generated by ydb-slo-action |
Switch to the new SLO Action's one-process-one-container model and push client-side latency aggregates instead of histograms. - metrics.py: compute p50/p95/p99 per push window via HdrHistogram and emit them as gauges; reset the histogram after each push so the values represent the last window only. Read ref/workload from the new WORKLOAD_REF/WORKLOAD_NAME env vars (with legacy REF/WORKLOAD fallback). Resolve the OTLP endpoint from the standard OTEL_EXPORTER_OTLP_* env vars before falling back to --otlp-endpoint. - docker-entrypoint.sh: tiny wrapper invoked by ydb-slo-action v2 that picks the *-create / *-run subcommand from WORKLOAD_NAME and forwards tuning flags from workload_current_command to *-run. - Dockerfile: install the entrypoint script; keep gcc/libc6-dev for the hdrhistogram sdist build. - slo.yml: use ydb-platform/ydb-slo-action/init@v2 with the workload matrix, fix workload_baseline_image (was pointing at current). - slo-report.yml: align with the Java SDK workflow — drop the JS-based label removal in favour of `gh pr edit --remove-label`. - requirements.txt: pin hdrhistogram for the percentile computation.
The v2 SLO action runs both current and baseline images with the same command. The baseline checkout is on main, which still has the old `python ./tests/slo/src` entrypoint that expects a subcommand as the first arg — so passing tuning flags directly (e.g. --read-rps 1000) fails with 'argument subcommand: invalid choice'. Mirror the Java SDK setup: build the baseline image with the current PR's Dockerfile + tests/slo runner against the baseline ydb library. That way the runner-side contract (entrypoint script, metrics format, env vars) is identical for both images and only the SDK under test differs. Also fix the inline report/label-removal jobs to use pull_request context (github.run_id, github.event.pull_request.number) instead of workflow_run fields that are undefined for this trigger.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information