[serve] Add tracing support for Windows and enable test_tracing_utils#62821
[serve] Add tracing support for Windows and enable test_tracing_utils#62821abrarsheikh merged 11 commits intoray-project:masterfrom
Conversation
Signed-off-by: baohg153 <baohg.main@gmail.com> Signed-off-by: phmkhoi <phanhuynhminhkhoi@gmail.com> Signed-off-by: TriNguyen1208 <ductri0981@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a safe_remove_directory utility to handle Windows file locking issues during test cleanup and updates tracing tests to utilize a consistent exporter import path. Feedback was provided to improve the robustness of the safe_remove_directory function by addressing potential silent failures, ensuring idempotency, and reducing code duplication. Additionally, it is recommended to remove commented-out code in the load_spans function to improve readability.
Signed-off-by: baohg153 <baohg.main@gmail.com> Signed-off-by: phmkhoi <phanhuynhminhkhoi@gmail.com> Signed-off-by: TriNguyen1208 <ductri0981@gmail.com>
|
Hi @abrarsheikh @akyang-anyscale, Could you please take a look at this PR when you have a chance? All available CI checks have passed, though Buildkite premerge is still waiting to be triggered. Happy to make any changes based on your feedback. Thank you! |
Signed-off-by: baohg153 <baohg.main@gmail.com> Signed-off-by: phmkhoi <phanhuynhminhkhoi@gmail.com> Signed-off-by: TriNguyen1208 <ductri0981@gmail.com>
Signed-off-by: baohg153 <baohg.main@gmail.com> Signed-off-by: phmkhoi <phanhuynhminhkhoi@gmail.com> Signed-off-by: TriNguyen1208 <ductri0981@gmail.com>
Signed-off-by: baohg153 <baohg.main@gmail.com> Signed-off-by: phmkhoi <phanhuynhminhkhoi@gmail.com> Signed-off-by: TriNguyen1208 <ductri0981@gmail.com>
Signed-off-by: baohg153 <baohg.main@gmail.com> Signed-off-by: phmkhoi <phanhuynhminhkhoi@gmail.com> Signed-off-by: TriNguyen1208 <ductri0981@gmail.com>
…153/ray into fix-enable-tracing-windows
Signed-off-by: baohg153 <baohg.main@gmail.com> Signed-off-by: phmkhoi <phanhuynhminhkhoi@gmail.com> Signed-off-by: TriNguyen1208 <ductri0981@gmail.com> Signed-off-by: Nguyễn Đức Trí <168810923+TriNguyen1208@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit eaaf573. Configure here.
| component_type=ServeComponentType.REPLICA, | ||
| component_name="component_name", | ||
| component_id="component_id", | ||
| tracing_exporter_import_path=DEFAULT_TRACING_EXPORTER_IMPORT_PATH, |
There was a problem hiding this comment.
Test relies on env var to reach error path
Medium Severity
Removing tracing_exporter_import_path=DEFAULT_TRACING_EXPORTER_IMPORT_PATH from the test_missing_dependencies call to setup_tracing causes the test to depend on the RAY_SERVE_TRACING_EXPORTER_IMPORT_PATH env var being set externally (by Bazel). Without it, the env var defaults to "", and setup_tracing returns False at the tracing_exporter_import_path == "" check before ever reaching the if not trace: check that raises ImportError. The pytest.raises(ImportError) block then fails because no exception is raised. Unlike other tests that inherently need tracing enabled, this test specifically needs a non-empty import path to exercise the dependency-check error path.
Reviewed by Cursor Bugbot for commit eaaf573. Configure here.
|
Hi @abrarsheikh, Could you please take a look at this PR again when you have a chance? I have adjusted the code with your previous reviews, and all checks have passed. |
|
@TriNguyen1208 thanks for your contribution, I see you have access to a windows machine, if you wish to maintain serve for windows please reach out over slack, there are many other tests in the repo we are skipping for windows purely because the team doesn't have the bandwidth to maintain it. LMK. |


Description
This PR enables
test_tracing_utilsfor Windows by addressing file locking issues and cross-platform line ending discrepancies. It also ensures that tracing tests are no longer skipped on Windows.Key Changes
pytest.mark.skipiffor thewin32platform to allow tracing tests to run in Windows environments.open()call that was never explicitly closed, causing aPermissionErroron Windows when attempting to delete span files after tests. Fixed by subclassing ConsoleSpanExporter with ashutdown()method that explicitly flushes and closes the file handle.provider.shutdown()is now called insidesafe_remove()to trigger the cleanup chain before deletion..split("}\n{")withre.split(r'}\s*\n\s*{', file_contents)inload_spans. This regex-based approach correctly handles different newline sequences (\nvs\r\n) and potential whitespace variations between JSON objects, preventing decoding failures.tracing_exporter_import_path=DEFAULT_TRACING_EXPORTER_IMPORT_PATHacross multiple test cases to ensure consistent tracing configuration.Related issues
Closes #61443
Additional information
shutil.rmtreeandos.removewithsafe_remove_directoryfor span directory management.load_spansfix is critical for Windows whereConsoleSpanExportermay output CRLF (\r\n) line endings, which previously broke the manual JSON string splitting logic.Test Verification
pytest -v python/ray/serve/tests/test_tracing_utils.pytest_tracing_utils.pypass locally on Windows after applying these changes.