-
Notifications
You must be signed in to change notification settings - Fork 24
Add generic xy endpoint in the server #203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add generic xy endpoint in the server #203
Conversation
1f8d158 to
071955b
Compare
1f89a7b to
48e8d11
Compare
|
Thanks for the contribution. I'll review it and provide feedback. Due to the size and impact on APIs it will take some days. |
bhufmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review comment for this.
| * @return an {@link GenericView} with the results | ||
| */ | ||
| @POST | ||
| @Path("/BarChart/{outputId}/tree") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the new endpoint should be about new xy endpoint where the x-axis is not the trace time. The chart type is part of the DisplayType. We could have other xy-charts like line, scatter etc. with a time-less x-axis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to genericXY.
| * @return an {@link GenericView} with the results | ||
| */ | ||
| @POST | ||
| @Path("/BarChart/{outputId}/bars") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the new endpoint should be about new xy endpoint where the x-axis is not the trace time. The chart type is part of the DisplayType. We could have other xy-charts like line, scatter etc. with a time-less x-axis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to genericXY.
e650ab3 to
e0eab17
Compare
bhufmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. It's clean and thoroughly implemented. I haven't done a full review though. I will do that once the mainline PR is approved (close to be approved). I added a comment what I noticed already.
| for (int i = 0; i < EXPECTED_ENTRIES.size(); i++) { | ||
| XyEntryStub expected = EXPECTED_ENTRIES.get(i); | ||
| XyEntryStub actual = actualEntries.get(i); | ||
| assertEquals("Entry ID mismatch at index " + i, expected.getId(), actual.getId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the ID is not always the same since it's a running counter for all data providers. So, you can check for a particular value. Same is for the parent ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
|
||
| // Validate fields | ||
| assertEquals("Name mismatch", "UNKNOWN_PID", seriesStub.getName()); | ||
| assertEquals("ID mismatch", 1, seriesStub.getId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the series ID is not always 1. It depends on how many other data providers were created in other tests before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I did a deeper review. I ran the tests successfully and was able to query the new endpoints. I added some comments. Please have a look. Thanks!
| /** | ||
| * Deserializer for Sampling that infers the type from array structure. | ||
| */ | ||
| public class SamplingDeserializer extends JsonDeserializer<ISampling> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed? This means that the client will provide the sampling as input parameters to http queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not. Removed.
| * @author Siwei Zhang | ||
| * @since 10.2 | ||
| */ | ||
| public class AxisDomainDeserializer extends JsonDeserializer<IAxisDomain> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed? This means that the client will provide the axis domain as input parameters to http queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not. Removed.
| @Operation(summary = "API to get the tree for generic xy chart", description = TREE_ENTRIES, responses = { | ||
| @ApiResponse(responseCode = "200", description = "Returns a list of generic xy chart entries. " + | ||
| CONSISTENT_PARENT, content = @Content(schema = @Schema(implementation = XYTreeResponse.class))), | ||
| @ApiResponse(responseCode = "400", description = INVALID_PARAMETERS, content = @Content(schema = @Schema(implementation = String.class))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just merged a PR #208 that fixes the error handling and when an error happens an ErrorResponse is returned instead of String. Please update the swagger doc accordingly for all errors. After rebasing check other implementations on how to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's all marked in other comments? I don't see other changes apart from the parts you already commented.
| @ApiResponse(responseCode = "200", description = "Returns a list of generic xy chart entries. " + | ||
| CONSISTENT_PARENT, content = @Content(schema = @Schema(implementation = XYTreeResponse.class))), | ||
| @ApiResponse(responseCode = "400", description = INVALID_PARAMETERS, content = @Content(schema = @Schema(implementation = String.class))), | ||
| @ApiResponse(responseCode = "404", description = PROVIDER_NOT_FOUND, content = @Content(schema = @Schema(implementation = String.class))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErrorResponse.class instead of String.class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| CONSISTENT_PARENT, content = @Content(schema = @Schema(implementation = XYTreeResponse.class))), | ||
| @ApiResponse(responseCode = "400", description = INVALID_PARAMETERS, content = @Content(schema = @Schema(implementation = String.class))), | ||
| @ApiResponse(responseCode = "404", description = PROVIDER_NOT_FOUND, content = @Content(schema = @Schema(implementation = String.class))), | ||
| @ApiResponse(responseCode = "405", description = NO_PROVIDER, content = @Content(schema = @Schema(implementation = String.class))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErrorResponse.class instead of String.class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| .setCategory(outputId).build()) { | ||
| TmfExperiment experiment = ExperimentManagerService.getExperimentByUUID(expUUID); | ||
| if (experiment == null) { | ||
| return Response.status(Status.NOT_FOUND).entity(NO_SUCH_TRACE).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ErrorResponseUtil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| Map<String, Object> params = queryParameters.getParameters(); | ||
| String errorMessage = QueryParametersUtil.validateGenericXYQueryParameters(params); | ||
| if (errorMessage != null) { | ||
| return Response.status(Status.BAD_REQUEST).entity(errorMessage).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ErrorResponseUtil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
|
||
| if (provider == null) { | ||
| // The analysis cannot be run on this trace | ||
| return Response.status(Status.METHOD_NOT_ALLOWED).entity(NO_PROVIDER).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ErrorResponseUtil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| @Parameter(description = OUTPUT_ID) @PathParam("outputId") String outputId, | ||
| @RequestBody(description = "Query parameters to fetch the xy model. " + TIMERANGE + " " + ITEMS_XY, content = { | ||
| @Content(examples = @ExampleObject("{\"parameters\":{" + TIMERANGE_EX + "," + ITEMS_EX + | ||
| "}}"), schema = @Schema(implementation = RequestedQueryParameters.class)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequestedQueryParameters with swagger to generate the TSP API.yaml specification is not sufficient anymore and needs to be update or a new one needs to be introduced to reflect the new input parameters for this. Please update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created new class (GenericXYQueryParameters and GenericTimeRange) for it.
| @Produces(MediaType.APPLICATION_JSON) | ||
| @Operation(summary = "API to get the xy model", description = "Unique endpoint for all xy models, " + | ||
| "ensures that the same template is followed for all endpoints.", responses = { | ||
| @ApiResponse(responseCode = "200", description = "Return the queried xy response", content = @Content(schema = @Schema(implementation = XYResponse.class))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not fully checked but I think the XYResponse / XYModel / SeriesModel class that are used to create the TSP API.yaml specification need to be updated or (new ones created) to reflect the updated models returned (regarding sampling, axis description etc).
You can check the generated yaml file by typing http://localhost:8080/tsp/api/openapi.yaml in your browser with the updated server running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified directly inside those classes considering those classes are shared with legacy xy in code.
e0eab17 to
bf42b95
Compare
bhufmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-reviewed this PR. I was able to test and use the new server with existing front-end code. I found a couple of minor things mostly regarding swagger doc.
| @JsonSubTypes.Type(value = AxisDomainCategorical.class, name = "categorical"), | ||
| @JsonSubTypes.Type(value = AxisDomainTimeRange.class, name = "timeRange") | ||
| }) | ||
| public interface IAxisDomain { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For swagger documentation, we decided to not use the usual Java syntax for interfaces, i.e. Interfaces start with an I. So, change it here to AxisDomain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| ISampling.CategorySampling.class, | ||
| ISampling.RangeSampling.class | ||
| }) | ||
| public interface ISampling { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For swagger documentation, we decided to not use the usual Java syntax for interfaces, i.e. Interfaces start with an I. So, change it here to Sampling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| static final String MARKER_CATEGORIES_EX = "\"" + REQUESTED_MARKER_CATEGORIES_KEY + "\": [\"category1\", \"category2\"]"; //$NON-NLS-1$ //$NON-NLS-2$ | ||
| static final String MARKER_SET_EX = "\"" + REQUESTED_MARKER_SET_KEY + "\": \"markerSetId\","; //$NON-NLS-1$ //$NON-NLS-2$ | ||
| static final String TIMERANGE_EX = "\"" + REQUESTED_TIMERANGE_KEY + "\": {\"start\": 111111111, \"end\": 222222222, \"nbTimes\": 1920}"; //$NON-NLS-1$ //$NON-NLS-2$ | ||
| static final String TIMERANGE_EX = "\"" + REQUESTED_TIMERANGE_KEY + "\": {\"start\": 111111111, \"end\": 222222222, \"nbTimes/nbSamples\": 1920}"; //$NON-NLS-1$ //$NON-NLS-2$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This examples is used in different places. nbSamples is not always applicable. I thing the example should be per use case and show nbTimes or nbSamples where applicable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| /** | ||
| * Property names below use underscores as per trace-server protocol. | ||
| */ | ||
| interface RequestedParameters { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one has to have a different name because there already another interface with that name: RequestedQueryParameters.RequesteParameters. When I checked the generated yaml file, I only see one in the file. For me, it was RequestedQueryParameters.RequesteParameters, but I think that's random and could be the other way round.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to GenericXYQueryParameters. Now there are both RequesteParameters and GenericXYQueryParameters generated in the yaml.
| * - Categories → array of strings: ["Read", "Write"] | ||
| * - TimeRanges → array of arrays: [[1, 2], [2, 3]] | ||
| */ | ||
| public class ISamplingSerializer extends StdSerializer<ISampling> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be called SamplingSerializer because it's not an interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| * @author Siwei Zhang | ||
| * @since 10.2 | ||
| */ | ||
| public class IAxisDomainSerializer extends JsonSerializer<IAxisDomain> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be called AxisDomainSerializer because it's not an interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
bf42b95 to
4beeedd
Compare
4beeedd to
109d9f5
Compare
PatrickTasse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit header should be updated since it no longer introduces a /bars endpoint.
| * the mutable map of query parameters | ||
| * @return an error message if validation fails, or null otherwise | ||
| */ | ||
| private static String validateRequestedTimeRangeWithSamples(Map<String, Object> params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just have a method that checks validity of requested_timerange (presence and its 3 fields) but does not convert it to requested_times like the other method does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| return MISSING_PARAMETERS + SEP + REQUESTED_TIMERANGE_KEY; | ||
| } | ||
|
|
||
| if(!(requestedTimeRange instanceof Map)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after if (global comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| * @return an {@link GenericView} with the results | ||
| */ | ||
| @POST | ||
| @Path("/genericXY/{outputId}/tree") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment, I would suggest to move the method so that both /genericXY/ endpoint methods are together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
|
||
| @JsonProperty("requested_timerange") | ||
| @Schema(requiredMode = RequiredMode.REQUIRED) | ||
| GenericTimeRange getRequestedTimeRange(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a generic time range that might not be timestamps, I would have suggested just a requested_range of type Range. But this would require corresponding rework in Trace Compass data provider parameter / fetch parameter utils.
Now we have two flavors of requested_timerange, (start, end, nbTimes) vs. (start, end, nbSamples), so it might not be a bad idea to use two different properties instead, e.g. requested_timerange vs. requested_range...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requested_timerange will always represent a time range. The only difference is that it uses nbSamples instead of nbTimes, indicating that the sampling points may not be time-based. Considering that they always represent time range, I think it would be better to keep calling it requested_timerange? It'll also be possible to let all usage of requested_timerange to use nbTimes later.
Those are the descriptions for it:
startwould be the start time in the trace to get the data from, e.g. trace start timeendwould be the end time in the trace to get the data from, e.g trace end timenbSampleswould be in this case the number of buckets to return. Then the back-end will be able to fill the buckets accordingly and the bucket range is determined by the corresponding duration in the range. The Front-end will decide the number of samples. For example, if the view width it 1000 pixels and the it would ask for 1000 samples, then the bucket width would be 1 pixel. If it wants to display a bigger bucket width (e.g. 10 pixels) it would ask for 100 buckets for the same 1000 view pixels.
This commit uses the newly introduced data provider type to support "bar chart" views. [Added] /genericXY/<id>/trees endpoint for generic xy view [Added] /genericXY/<id>/xy endpoint for generic xy view [Fixed] All request parameter "requested_timerange" with no number of times specified
109d9f5 to
3af3883
Compare
I believe all comments were addressed
bbde152
into
eclipse-tracecompass-incubator:master
What it does
This series of commits add the new
/BarChartendpoint proposed in:eclipse-cdt-cloud/trace-server-protocol#114
It requires the introduced bar chart data provider implemented in:
eclipse-tracecompass/org.eclipse.tracecompass#291
CallStackAnalysisis used as the initial example input to demonstrate this functionality through a function density view.How to test
You can test the new bar chart data provider endpoints using the following sequence of HTTP requests.
Open trace
Create an experiment
(optional) Check bar chart provider availability
Get tree and axis descriptions
Get bar chart data
Follow-ups
Review checklist