Add CLI argument for server-metrics-url #39
Conversation
… unreachable (#47) * Fix exception thrown by genai-perf if metrics url is unreachable * Fix comments * Fix comments * Fix comments * Add space in error message
lkomali
force-pushed
the
lkomali-tpa-275
branch
from
3696382
to
91bb0dc
Compare
genai-perf/tests/test_cli.py
Outdated
| @@ -917,3 +917,75 @@ def test_get_extra_inputs_as_dict(self, extra_inputs_list, expected_dict): | |||
| namespace.extra_inputs = extra_inputs_list | |||
| actual_dict = parser.get_extra_inputs_as_dict(namespace) | |||
| assert actual_dict == expected_dict | |||
|
|
|||
| TEST_TRITON_METRICS_URL = "http://custom-metrics-url:8002/metrics" | |||
There was a problem hiding this comment.
Is there a reason for creating these global variables? I feel like this could be confusing. I'd need to scroll down quite a bit to see why they are used. It'd be better to have these as local variables inside the one test.
genai-perf/tests/test_cli.py
Outdated
| ): | ||
| monkeypatch.setattr("sys.argv", args_list) | ||
|
|
||
| if expected_error: |
There was a problem hiding this comment.
When you start using branching logic like this, it suggests this should be two tests. Do you want to add the valid test to the test_cli valid arg test and then have a separate one just for the error testing? That assumes there's not a generic error test that you can reuse... which if we don't have, we should refactor sometime soon to add.
There was a problem hiding this comment.
we have test_conditional_errors, that handles error scenarios.
We can use it for the invalid url case, I think
This is a new PR for adding a CLI argument to get server-metrics-url to collect telemetry data when the server is running on a different machine than genai-perf.