Refactor for otel.#824
Conversation
mhdirkse
left a comment
mhdirkse
left a comment
There was a problem hiding this comment.
This is not my complete review yet, but here are a few comments already.
| <artifactId>opentelemetry-exporter-otlp</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.opentelemetry.proto</groupId> |
There was a problem hiding this comment.
I would like these dependencies to be in the top-level pom.xml. There they should appear in <dependencyManagement>. This way the versions of the dependencies are forced to be the same for each subproject. Then the version can be omitted in ladybug-common/pom.xml.
| } | ||
| case 'copyReport': { | ||
| this.copyReport(); | ||
| if (this.rerunnable) { |
There was a problem hiding this comment.
Two comments:
- You are implementing here that non-rerunnable reports cannot be copied to the test tab. I am not sure whether this is the restriction we need. An alternative could be that copying to the test tab is allowed but rerunning is not.
- Class ReportComponent implements only the report component for separate tabs. It does not apply to the report component within the debug tab. Please work with class
ReportSharedStrategy. This is common code for handling reports both in separate report tabs and in the debug tab. - Please work with HierarchicalReport here. I intend to replace Report by HierarchicalReport.
# Conflicts: # ladybug-frontend/src/app/report/checkpoint-metadata-table/checkpoint-metadata-table.spec.ts # ladybug-frontend/src/app/report/checkpoint-value/checkpoint-value.component.spec.ts # ladybug-frontend/src/app/report/report-metadata-table/report-metadata-table.spec.ts # ladybug-frontend/src/app/report/report-value/report-value.component.spec.ts # ladybug-frontend/src/app/report/report.component.ts
| } | ||
|
|
||
| private copyReport(): void { | ||
| if (!this.nodeValueState?.rerunnable) { |
There was a problem hiding this comment.
This hides the option that rerunnable is undefined. If rerunnable is undefined I would like to see an error in the browser console to indicate a programming error. This is different from the case that rerunnable is false.
|
|
||
| private rerunReport(): void { | ||
| if (!this.nodeValueState?.rerunnable) { | ||
| this.toastService.showWarning('This report cannot be rerun'); |
There was a problem hiding this comment.
Same point as for copyReport(). I would like an error in the browser console when rerunnable is undefined - that would be a programming error that would be relevant for testers of Ladybug.
| stubStrategy: string; | ||
| transformation: string; | ||
| variables: string; | ||
| rerunnable: boolean; |
There was a problem hiding this comment.
When non-rerunnable reports cannot be copied to the test tab, then it is probably sufficient to have the rerunnable boolean in HierarchicalReport. Can you investigate?
| testTool.endpoint(correlationId, null, reportName, "\r"); | ||
| } | ||
| reportNames.add(reportName = "OpenTelemetry report"); | ||
| if (reportName.equals(createReportAction)) { |
There was a problem hiding this comment.
Good to have this. Now I can test without the test environment with Docker and Kubernetes that you used for realistic OpenTelemetry data.
| <artifactId>opentelemetry-api</artifactId> | ||
| <version>1.62.0</version> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
Good that these dependencies are in the parent pom.xml
| <groupId>org.projectlombok</groupId> | ||
| <artifactId>lombok</artifactId> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
Good. The versions are inherited from the parent pom.xml.
| private @Setter @Getter long estimatedMemoryUsage; | ||
| private @Setter @Getter String correlationId; | ||
| private @Setter @Getter Map<String, String> variables; | ||
| private @Setter @Getter boolean rerunnable; |
There was a problem hiding this comment.
Good. Not nullable because it is a primitive type. It is also not nullable in the frontend.
| @@ -1,110 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
Good that this class is gone. I understood from your thesis that having this custom class violated the OpenTelemetry standard.
| return checkpoint(correlationId, childThreadId, sourceClassName, name, | ||
| message, null, stubableCode, stubableCodeThrowsException, | ||
| matchingStubStrategies, checkpointType, levelChangeNextCheckpoint); | ||
| matchingStubStrategies, checkpointType, levelChangeNextCheckpoint, true); |
There was a problem hiding this comment.
Reviewing your changes in class TestTool requires a bit more effort than reviewing the other changes. When a bug is introduced here it has more impact then bugs in other code. I also would like Jaco to review this before merging this PR.
| <void property="path"> | ||
| <string>path</string> | ||
| </void> | ||
| <void property="rerunnable"> |
There was a problem hiding this comment.
There are many files in src/test/resources for expected values of reports. Only one of them changes. This indicates that importing and exporting non-rerunnable reports is not properly covered by the tests. Please improve the tests.
Doing this introduces a conflict with a PR needed by one of our customers. The time needed for that other PR was billed to NN, suggesting it has precedence over the present PR. I will contact Jaco and our service manager Vivienne about this conflict.
2 participants
General refactor to prepare Ladybug for OpenTelemetry: