test(otel): cover OTEL_METRIC_EXPORT_TIMEOUT parametrically - #7682
test(otel): cover OTEL_METRIC_EXPORT_TIMEOUT parametrically#7682vpellan wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ecff7c4ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| library.otel_get_meter(meter_name, "1.0.0", "", {}) | ||
| library.otel_create_counter(meter_name, instrument_name, "1", "Metric SDK initialization") | ||
| library.otel_counter_add(meter_name, instrument_name, "1", "Metric SDK initialization", 1, {}) | ||
| library.otel_metrics_force_flush() |
There was a problem hiding this comment.
When the minimum-finite parameter sets OTEL_METRIC_EXPORT_TIMEOUT=1, this force flush may time out before the metric crosses the container network; its Boolean result is ignored, and the subsequent wait_for_num_otlp_metrics(num=1) then fails after eight seconds without ever checking telemetry. Initialize the metrics SDK without making test progress depend on an export governed by the value being inspected.
Useful? React with 👍 / 👎.
| def test_unset_and_empty_use_default(self, test_agent: TestAgentAPI, test_library: APMLibrary) -> None: | ||
| config = _metric_export_timeout_configuration(test_agent, test_library) | ||
| assert config.get("origin") == "default" | ||
| assert _metric_export_timeout(config) >= 0 |
There was a problem hiding this comment.
For the unset and empty inputs, this accepts every nonnegative timeout, including an erroneous fallback such as 0 or 1, so the test can pass even though its name says the default is used. The commit already identifies the expected defaults as 30000 ms for PHP and 7500 ms for the other enabled SDKs; assert those exact values, with separately manifest-activated methods if necessary. The identical >= 0 check in test_invalid_values_use_default has the same false-pass behavior.
Useful? React with 👍 / 👎.
| tests/otel_tracing_e2e/test_e2e.py::Test_OTelLogE2E: irrelevant | ||
| tests/otel_tracing_e2e/test_e2e.py::Test_OTelMetricE2E: irrelevant | ||
| tests/otel_tracing_e2e/test_e2e.py::Test_OTelTracingE2E: irrelevant | ||
| tests/parametric/otel_env_vars/test_otel_metric_export_timeout.py::Test_OTEL_METRIC_EXPORT_TIMEOUT: v1.23.0 |
There was a problem hiding this comment.
That entry probably should be incomplete_test_app (/metrics/otel/force_flush endpoint is not implemented)
|
|
||
| STABLE_VALUES = [ | ||
| pytest.param({**DEFAULT_ENVIRONMENT, "OTEL_METRIC_EXPORT_TIMEOUT": "0"}, 0, id="zero-unlimited"), | ||
| pytest.param({**DEFAULT_ENVIRONMENT, "OTEL_METRIC_EXPORT_TIMEOUT": "1"}, 1, id="minimum-finite"), |
There was a problem hiding this comment.
Maybe testing for 1 is not necessary if we test a bigger value. It might be flaky
| @pytest.mark.parametrize("library_env", INVALID_VALUES) | ||
| def test_invalid_values_use_default(self, test_agent: TestAgentAPI, test_library: APMLibrary) -> None: | ||
| config = _metric_export_timeout_configuration(test_agent, test_library) | ||
| assert config.get("origin") == "default" |
There was a problem hiding this comment.
I'm not exactly sure why it is set "env_var" instead of "origin" in practice.
Is there a way to check this differently ?
2 participants
Motivation
Add specification-based parametric coverage for
OTEL_METRIC_EXPORT_TIMEOUT.Tracks APMAPI-2397.
Changes
precedence.
Workflow
R&P team
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛏 #apm-shared-testing 🛏
Reviewer checklist
tests/ormanifests/is modified? I have the approvalfrom R&P team
build-XXX-imagelabel is presentR&P team