Add telemetry support#20
Conversation
that1guy15
left a comment
that1guy15
left a comment
There was a problem hiding this comment.
Few NIT comments but overall look great.
The one thing missing is a document in /docs/example-scripts with a walk-through of its usage. I usually use the same code from scripts and add some comentery around it. This is optional for this PR
| from .resources import AosResources | ||
| from .external_systems import AosExternalSystems | ||
|
|
||
| from .telemetry import AosTelemetryManager |
There was a problem hiding this comment.
Is there a reason for both imports? line 8 and 15
There was a problem hiding this comment.
removed the one in line 8
|
|
||
| @classmethod | ||
| def from_json (cls, d:dict): | ||
| return AosTelemetryEndpointStatus( connected=d.get("connected"), connection_log=d.get("connectionLog"), connection_time=d.get("connectionTime"), last_tx_time=d.get("lastTransmitedTime"),epoch=d.get("epoch"),connection_reset_count=d.get("connectionResetCount"), dns_log=d.get("dnsLog"), disconnection_time=d.get("disconnectionTime")) |
There was a problem hiding this comment.
need to wrap this line please. Suggested approach is to use black (installed in the venv) on all files you work with before commiting.
|
|
||
| @classmethod | ||
| def from_json(cls, d:dict): | ||
| return AosTelemetryEndpoint(id=d.get("id"), host=d.get("host"), port=d.get("port"), streaming_type=d.get("streaming_type"), protocol=d.get("protocol"),sequencing_mode=d.get("sequencing_mode"), ep_status=AosTelemetryEndpointStatus.from_json(d.get("status"))) |
| """ | ||
| Telemetry manager class used to manage the telemetry endpoints | ||
| """ | ||
| def add_endpoint(self, host, port, streaming_type, protocol = "protoBufOverTcp", mode = "sequenced"): |
There was a problem hiding this comment.
please provide an output type for all functions/methods and their arguments.
https://docs.python.org/3/library/typing.html
| "sequencing_mode": mode, | ||
| "protocol": protocol, | ||
| } | ||
| self.rest.json_resp_post(uri = "/api/streaming-config", data = body) |
There was a problem hiding this comment.
I dont see a return here. We need to give some response even if its just returning the output of the post.
| Get the existing streaming endpoints as AosTelemetryEndpoint objects | ||
| """ | ||
| r = self.rest.json_resp_get(uri = "/api/streaming-config") | ||
| return [AosTelemetryEndpoint.from_json(i) for i in r['items'] ] |
There was a problem hiding this comment.
This assumes you will get a payload with ["items"] present.
Either set a try/except for return codes or validate r["items"] is present with an if/else.
There was a problem hiding this comment.
In other areas we have null() data class for a blank object or one that is not returned. This would be a good option that lines up with the rest of the code base. ex
apstra-api-python/aos/blueprint.py
Line 91 in 0366488
| id | ||
| (str) id of the endpoint | ||
| """ | ||
| self.rest.delete(uri="/api/streaming-config/"+id) |
There was a problem hiding this comment.
return self.rest.delete()
| """ | ||
| eps = self.get_endpoints() | ||
| for ep in eps: | ||
| self.delete_endpoint(ep.id) |
| mock_rest = mock.Mock() | ||
| mgr = AosTelemetryManager(mock_rest) | ||
|
|
||
| mock_rest.json_resp_get.return_value = { |
There was a problem hiding this comment.
I suggest moving this to a fixture so we can track it with the right Apostra version.
There are fixture tools in tests/utils.py to use.
|
|
||
| def test_add_endpoint(): | ||
| mgr.add_endpoint("fakehost", "fakeport", "faketype") | ||
| mock_rest.json_resp_post.assert_called_with(uri='/api/streaming-config', data={'hostname': 'fakehost', 'port': 'fakeport', 'streaming_type': 'faketype', 'sequencing_mode': 'sequenced', 'protocol': 'protoBufOverTcp'}) |
There was a problem hiding this comment.
wrap this line please. Suggest running black on all files you worked with.
There was a problem hiding this comment.
please address this comment.
that1guy15
left a comment
that1guy15
left a comment
There was a problem hiding this comment.
Looks good. please address comments on test file.
|
|
||
| def test_add_endpoint(): | ||
| mgr.add_endpoint("fakehost", "fakeport", "faketype") | ||
| mock_rest.json_resp_post.assert_called_with(uri='/api/streaming-config', data={'hostname': 'fakehost', 'port': 'fakeport', 'streaming_type': 'faketype', 'sequencing_mode': 'sequenced', 'protocol': 'protoBufOverTcp'}) |
There was a problem hiding this comment.
please address this comment.
| mock_rest = mock.Mock() | ||
| mgr = AosTelemetryManager(mock_rest) | ||
|
|
||
| mock_rest.json_resp_get.return_value = { |
| r = mgr.get_endpoints() | ||
| assert len(r) == 3 | ||
| mock_rest.json_resp_get.assert_called() | ||
|
|
There was a problem hiding this comment.
tox -e py38 flake8 fails multiple lines here. Please address and make sure it passes.
flake8 run-test: commands[0] | flake8 aos/ tests/
tests/test_telemetry.py:2:1: F401 'telnetlib.AO' imported but unused
tests/test_telemetry.py:6:1: F401 'unittest.mock.call' imported but unused
tests/test_telemetry.py:9:1: F401 'tests.util.make_session' imported but unused
tests/test_telemetry.py:12:1: E302 expected 2 blank lines, found 0
tests/test_telemetry.py:130:1: E302 expected 2 blank lines, found 1
tests/test_telemetry.py:132:86: E501 line too long (220 > 85 characters)
tests/test_telemetry.py:134:1: E302 expected 2 blank lines, found 1
tests/test_telemetry.py:138:1: E302 expected 2 blank lines, found 1
tests/test_telemetry.py:144:1: W391 blank line at end of file
ERROR: InvocationError for command /Users/boothr/apstra-api-python/.tox/flake8/bin/flake8 aos/ tests/ (exited with code 1)
There was a problem hiding this comment.
Which has me curious why the GitHub actions to run these test don't fail. Ill dig into this.
…python into add_telemetry_support
3 participants
No description provided.