surahman commented on a change in pull request #3786:
URL: https://github.com/apache/incubator-heron/pull/3786#discussion_r835616292
##########
File path: heron/tools/common/src/python/clients/tracker.py
##########
@@ -54,8 +54,8 @@
SCHEDULER_LOCATION_URL_FMT = "%s/schedulerlocation" % TOPOLOGIES_URL_FMT
METRICS_URL_FMT = "%s/metrics" % TOPOLOGIES_URL_FMT
-METRICS_QUERY_URL_FMT = "%s/metricsquery" % TOPOLOGIES_URL_FMT
-METRICS_TIMELINE_URL_FMT = "%s/metricstimeline" % TOPOLOGIES_URL_FMT
+METRICS_QUERY_URL_FMT = "%s/metrics/query" % TOPOLOGIES_URL_FMT
+METRICS_TIMELINE_URL_FMT = "%s/metrics/timeline" % TOPOLOGIES_URL_FMT
Review comment:
Nitpick:
```suggestion
METRICS_QUERY_URL_FMT = METRICS_URL_FMT % "%s/query" %
TOPOLOGIES_URL_FMT
METRICS_TIMELINE_URL_FMT = METRICS_URL_FMT % "%s/timeline" %
TOPOLOGIES_URL_FMT
```
##########
File path: heron/tools/tracker/src/python/query_operators.py
##########
@@ -476,7 +477,7 @@ async def execute(self, tracker, tmanager:
TManagerLocation, start: int, end: in
# Initialize with first metrics timeline and its instance
met = Metrics(None, None, metric.instance, start, end,
metric.timeline.copy())
for timestamp in list(met.timeline.keys()):
- v = self._f(met.timeline[timestamp],
metrics2[""].timeline.get(timestamp))
+ v = self._f(met.timeline[timestamp], metrics2[""].timeline[timestamp])
Review comment:
Good change. It is better to use the `[]` instead of the `.get()` method
throughout to access the dictionary elements for consistency.
##########
File path: heron/tools/common/src/python/clients/tracker.py
##########
@@ -122,27 +122,26 @@ def strip_whitespace(s):
backpressure=backpressure
)
-def api_get(url: str, params=None) -> dict:
+def api_get(url: str, params=None) -> Any:
"""Make a GET request to a tracker URL and return the result."""
start = time.time()
try:
+ Log.debug(f"Requesting URL: {url} with params: {params}")
response = requests.get(url, params)
response.raise_for_status()
except Exception as e:
- Log.error(f"Unable to get response from {url}: {e}")
+ Log.error(f"Unable to get response from {url} with params {params}: {e}")
return None
end = time.time()
data = response.json()
- if data["status"] != "success":
- Log.error("error from tracker: %s", data["message"])
+ if response.status_code != requests.codes.ok:
+ Log.error("error from tracker: %s", response.status_code)
return None
- execution = data["executiontime"] * 1000
duration = (end - start) * 1000
- Log.debug(f"URL fetch took {execution:.2}ms server time for {url}")
Log.debug(f"URL fetch took {duration:.2}ms round trip time for {url}")
- return data["result"]
+ return data
Review comment:
This now returns the entire JSON response instead of just the `result`
tuple's value. We must make sure all upstream calls handle this return
correctly and unpack the tuple.
##########
File path: heron/tools/tracker/src/python/routers/metrics.py
##########
@@ -151,19 +152,19 @@ class TimelinePoint(BaseModel): # pylint:
disable=too-few-public-methods
class MetricsQueryResponse(BaseModel): # pylint: disable=too-few-public-methods
"""A metrics timeline over an interval."""
- start_time: int = Field(..., alias="starttime")
- end_time: int = Field(..., alias="endtime")
+ starttime: int = Field(..., alias="starttime")
+ endtime: int = Field(..., alias="endtime")
Review comment:
Would be best to switch these to `str` as well for consistency as you
have switched their similar definitions from `int` to `str` elsewhere? Or are
these used in some sort of numeric calculation somewhere?
##########
File path: heron/tools/tracker/src/python/routers/metrics.py
##########
@@ -151,19 +152,19 @@ class TimelinePoint(BaseModel): # pylint:
disable=too-few-public-methods
class MetricsQueryResponse(BaseModel): # pylint: disable=too-few-public-methods
"""A metrics timeline over an interval."""
- start_time: int = Field(..., alias="starttime")
- end_time: int = Field(..., alias="endtime")
+ starttime: int = Field(..., alias="starttime")
+ endtime: int = Field(..., alias="endtime")
timeline: List[TimelinePoint] = Field(
..., description="list of timeline point objects",
)
[email protected]("/metricsquery", response_model=MetricsQueryResponse)
[email protected]("/metrics/query", response_model=MetricsQueryResponse)
async def get_metrics_query( # pylint: disable=too-many-arguments
cluster: str,
- role: Optional[str],
environ: str,
query: str,
+ role: Optional[str] = None,
start_time: int = Query(..., alias="starttime"),
end_time: int = Query(..., alias="endtime"),
Review comment:
If you switch over to `str` in the class definition these will need to
be updated as well.
##########
File path: heron/tools/common/src/python/clients/tracker.py
##########
@@ -54,8 +54,8 @@
SCHEDULER_LOCATION_URL_FMT = "%s/schedulerlocation" % TOPOLOGIES_URL_FMT
METRICS_URL_FMT = "%s/metrics" % TOPOLOGIES_URL_FMT
-METRICS_QUERY_URL_FMT = "%s/metricsquery" % TOPOLOGIES_URL_FMT
-METRICS_TIMELINE_URL_FMT = "%s/metricstimeline" % TOPOLOGIES_URL_FMT
+METRICS_QUERY_URL_FMT = "%s/metrics/query" % TOPOLOGIES_URL_FMT
+METRICS_TIMELINE_URL_FMT = "%s/metrics/timeline" % TOPOLOGIES_URL_FMT
Review comment:
I also question whether these values should be located in a central
resource file so we can reference them for the request router URLs as well.
##########
File path: heron/tools/tracker/src/python/routers/container.py
##########
@@ -22,31 +22,34 @@
topology, particularly data about heron containers.
"""
+from asyncio import constants
+from subprocess import CompletedProcess
from typing import List, Optional
+from heron.common.src.python.utils.log import Log
from heron.proto import common_pb2, tmanager_pb2
from heron.tools.tracker.src.python import state, utils
-from heron.tools.tracker.src.python.utils import EnvelopingAPIRouter
import httpx
-from fastapi import Query
+# from fastapi import Query
+from fastapi import Query, APIRouter
from pydantic import BaseModel, Field
from starlette.responses import StreamingResponse
-router = EnvelopingAPIRouter()
+router = APIRouter()
[email protected]("/containerfiledata")
[email protected]("/container/filedata")
Review comment:
For better maintainability, it would be best to construct these URLs
from a central resource file. That way, if a URL needs to be updated we would
do it from one location and all updates would cascade without broken links.
##########
File path: heron/tools/tracker/src/python/app.py
##########
@@ -80,35 +79,23 @@ async def shutdown_event():
"""Stop recieving topology updates."""
state.tracker.stop_sync()
-
@app.exception_handler(Exception)
async def handle_exception(_, exc: Exception):
- payload = ResponseEnvelope[str](
- result=None,
- execution_time=0.0,
- message=f"request failed: {exc}",
- status=constants.RESPONSE_STATUS_FAILURE
- )
+ message=f"request failed: {exc}"
Review comment:
Whitespace fix:
```suggestion
message = f"request failed: {exc}"
```
##########
File path: heron/tools/tracker/src/python/app.py
##########
@@ -80,35 +79,23 @@ async def shutdown_event():
"""Stop recieving topology updates."""
state.tracker.stop_sync()
-
@app.exception_handler(Exception)
async def handle_exception(_, exc: Exception):
- payload = ResponseEnvelope[str](
- result=None,
- execution_time=0.0,
- message=f"request failed: {exc}",
- status=constants.RESPONSE_STATUS_FAILURE
- )
+ message=f"request failed: {exc}"
status_code = 500
if isinstance(exc, StarletteHTTPException):
status_code = exc.status_code
if isinstance(exc, RequestValidationError):
status_code = 400
- return JSONResponse(content=payload.dict(), status_code=status_code)
-
+ return JSONResponse(content=message, status_code=status_code)
[email protected]("/clusters", response_model=ResponseEnvelope[List[str]])
[email protected]("/clusters")
async def clusters() -> List[str]:
- return ResponseEnvelope[List[str]](
- execution_time=0.0,
- message="ok",
- status="success",
- result=[s.name for s in state.tracker.state_managers],
- )
-
+ return (s.name for s in state.tracker.state_managers)
+
Review comment:
Whitespace fix:
```suggestion
```
##########
File path: heron/tools/tracker/src/python/tracker.py
##########
@@ -89,7 +89,7 @@ def get_topology(
cluster: str,
role: Optional[str],
environ: str,
- topology_name: str,
+ topology_name: str,
Review comment:
Whitespace fix:
```suggestion
topology_name: str,
```
##########
File path: heron/tools/common/src/python/clients/tracker.py
##########
@@ -66,9 +66,9 @@
JMAP_URL_FMT = "%s/jmap" % TOPOLOGIES_URL_FMT
HISTOGRAM_URL_FMT = "%s/histo" % TOPOLOGIES_URL_FMT
-FILE_DATA_URL_FMT = "%s/containerfiledata" % TOPOLOGIES_URL_FMT
-FILE_DOWNLOAD_URL_FMT = "%s/containerfiledownload" % TOPOLOGIES_URL_FMT
-FILESTATS_URL_FMT = "%s/containerfilestats" % TOPOLOGIES_URL_FMT
+FILE_DATA_URL_FMT = "%s/container/filedata" % TOPOLOGIES_URL_FMT
+FILE_DOWNLOAD_URL_FMT = "%s/container/filedownload" % TOPOLOGIES_URL_FMT
+FILESTATS_URL_FMT = "%s/container/filestats" % TOPOLOGIES_URL_FMT
Review comment:
Nitpick:
```suggestion
CONTAINER_URL_FMT = "%s/container"
FILE_DATA_URL_FMT = CONTAINER_URL_FMT % "%s/filedata" %
TOPOLOGIES_URL_FMT
FILE_DOWNLOAD_URL_FMT = CONTAINER_URL_FMT % "%s/filedownload" %
TOPOLOGIES_URL_FMT
FILESTATS_URL_FMT = CONTAINER_URL_FMT % "%s/filestats" %
TOPOLOGIES_URL_FMT
```
##########
File path: heron/tools/tracker/src/python/metricstimeline.py
##########
@@ -73,21 +73,21 @@ async def get_metrics_timeline(
# Form and send the http request.
url = f"http://{tmanager.host}:{tmanager.stats_port}/stats";
- with httpx.AsyncClient() as client:
+ async with httpx.AsyncClient() as client:
result = await client.post(url,
data=request_parameters.SerializeToString())
# Check the response code - error if it is in 400s or 500s
- if result.code >= 400:
+ if result.status_code >= 400:
message = f"Error in getting metrics from Tmanager, code: {result.code}"
raise Exception(message)
# Parse the response from tmanager.
response_data = tmanager_pb2.MetricResponse()
response_data.ParseFromString(result.content)
- if response_data.status.status == common_pb2.NOTOK:
- if response_data.status.HasField("message"):
- Log.warn("Received response from Tmanager: %s",
response_data.status.message)
+ # if response_data.status.status == common_pb2.NOTOK:
+ # if response_data.status.HasField("message"):
+ # Log.warn("Received response from Tmanager: %s",
response_data.status.message)
Review comment:
Delete these commented out lines?
##########
File path: heron/tools/tracker/src/python/routers/container.py
##########
@@ -186,29 +189,30 @@ async def _get_exception_log_response(
@router.get("/exceptions", response_model=List[ExceptionLog])
async def get_exceptions( # pylint: disable=too-many-arguments
cluster: str,
- role: Optional[str],
environ: str,
component: str,
- instances: List[str] = Query(..., alias="instance"),
+ instances: Optional[List[str]] = Query(None, alias="instance"),
topology_name: str = Query(..., alias="topology"),
+ role: Optional[str] = None,
):
"""Return info about exceptions that have occurred per instance."""
exception_response = await _get_exception_log_response(
cluster, role, environ, component, instances, topology_name,
summary=False
)
-
- return [
- ExceptionLog(
- hostname=exception_log.hostname,
- instance_id=exception_log.instance_id,
- stack_trace=exception_log.stacktrace,
- lasttime=exception_log.lasttime,
- firsttime=exception_log.firsttime,
- count=str(exception_log.count),
- logging=exception_log.logging,
- )
- for exception_log in exception_response.exceptions
- ]
+ print(f"NICK: exception response: {exception_response}")
+
+ ret = []
+ for exception_log in exception_response.exceptions:
+ ret.append(ExceptionLog(
+ hostname = exception_log.hostname,
+ instance_id = exception_log.instance_id,
+ stacktrace = exception_log.stacktrace,
+ lasttime = exception_log.lasttime,
+ firsttime = exception_log.firsttime,
+ count = str(exception_log.count),
+ logging = exception_log.logging,
+ ))
Review comment:
Indentation and whitespace fix
```suggestion
ret.append(ExceptionLog(
hostname=exception_log.hostname,
instance_id=exception_log.instance_id,
stacktrace=exception_log.stacktrace,
lasttime=exception_log.lasttime,
firsttime=exception_log.firsttime,
count=str(exception_log.count),
logging=exception_log.logging,
))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
