api for uploading spans #302
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
skull8888888
requested changes
Jan 5, 2025
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 4db0162 in 54 seconds
More details
- Looked at
195lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. app-server/src/api/v1/queues.rs:72
- Draft comment:
Usingunwraponserde_json::to_valuecan cause a panic if serialization fails. Consider handling this error gracefully. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
For TraceType::EVENT, this is an enum value which should always be serializable - there's no realistic way this serialization could fail. The unwrap() here is actually fine since we have strong guarantees about the input. While technically the comment is correct that unwrap() can panic, in this specific case the risk is theoretical rather than practical.
I could be wrong about the enum serialization guarantees - there could be custom serialization implementations that could fail. Also, this establishes a pattern that could be problematic if copied elsewhere.
Even with custom serialization, basic enum variants should always be serializable. The pattern concern is valid but should be raised as a broader architectural discussion rather than a PR comment.
This comment should be removed as the unwrap() usage here is actually safe given the input is a simple enum variant that cannot fail to serialize.
2. app-server/src/api/v1/queues.rs:81
- Draft comment:
Usingunwraponserde_json::to_valuecan cause a panic if serialization fails. Consider handling this error gracefully. - Reason this comment was not posted:
Marked as duplicate.
3. app-server/src/api/v1/queues.rs:104
- Draft comment:
span_idis of typeUuidwhich implementsCopy, soclone()is unnecessary. You can use*span_idinstead. - Reason this comment was not posted:
Confidence changes required:50%
The use ofcloneonspan_idin themapfunction is unnecessary sinceUuidisCopy.
4. frontend/app/api/projects/[projectId]/queues/[queueId]/remove/route.ts:33
- Draft comment:
Good use ofnonempty()to ensureaddedLabelsis not empty. - Reason this comment was not posted:
Confidence changes required:0%
Thenonempty()method ensures that the array is not empty, which is a good validation step.
5. frontend/app/api/projects/[projectId]/queues/[queueId]/remove/route.ts:73
- Draft comment:
Good use of optional chaining withaction?.resultIdto safely accessresultId. - Reason this comment was not posted:
Confidence changes required:0%
The optional chaining operator is used correctly to safely access properties ofaction.
6. frontend/app/api/projects/[projectId]/queues/[queueId]/remove/route.ts:122
- Draft comment:
Good use of optional chaining withaction?.datasetIdto safely accessdatasetId. - Reason this comment was not posted:
Confidence changes required:0%
The optional chaining operator is used correctly to safely access properties ofaction.
Workflow ID: wflow_y21Plam4uXedb3YU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
skull8888888
reviewed
Jan 6, 2025
app-server/src/api/v1/queues.rs
Outdated
| } | ||
|
|
||
| #[post("/queues/upload")] | ||
| async fn upload_to_queue( |
There was a problem hiding this comment.
minor nit, but I would say push is more fitting than upload
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b5d08f7 in 38 seconds
More details
- Looked at
52lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. frontend/app/api/projects/[projectId]/queues/[queueId]/remove/route.ts:68
- Draft comment:
Consider using parameterized queries or a safer method for SQL string interpolation to prevent potential SQL injection vulnerabilities. - Reason this comment was not posted:
Comment was on unchanged code.
2. frontend/app/api/projects/[projectId]/queues/[queueId]/remove/route.ts:75
- Draft comment:
Add null checks foractionbefore accessing its properties to prevent potential runtime errors. This applies to other instances whereactionis accessed without checks. - Reason this comment was not posted:
Comment was on unchanged code.
3. frontend/app/api/projects/[projectId]/queues/route.ts:15
- Draft comment:
Consider adding validation for thenamefield to ensure it is present and valid before proceeding with the database operation. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_SomIx4GX9J6Qci4Z
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 10621ee in 14 seconds
More details
- Looked at
29lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. app-server/src/api/v1/queues.rs:44
- Draft comment:
The endpoint has been changed from/queues/uploadto/queues/push, but the PR description still mentions/queues/upload. Please update the description for consistency. - Reason this comment was not posted:
Confidence changes required:50%
The PR changes the endpoint from/queues/uploadto/queues/push, but the description still mentions/queues/upload. This inconsistency should be addressed.
2. app-server/src/main.rs:431
- Draft comment:
The function name has been changed fromupload_to_queuetopush_to_queue, but the PR description still mentionsupload_to_queue. Please update the description for consistency. - Reason this comment was not posted:
Confidence changes required:50%
The PR changes the function name fromupload_to_queuetopush_to_queue, but the description still mentionsupload_to_queue. This inconsistency should be addressed.
Workflow ID: wflow_2JCzAcPFFH7f35BT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
skull8888888
reviewed
Jan 7, 2025
app-server/src/api/v1/queues.rs
Outdated
|
|
||
| #[derive(Deserialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| struct UploadItem { |
There was a problem hiding this comment.
should rename here and below too
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2e20af5 in 18 seconds
More details
- Looked at
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. app-server/src/api/v1/queues.rs:24
- Draft comment:
RenamingUploadItemtoPushItemis consistent with the new endpoint namepush_to_queue. This aligns with the intent of the PR. - Reason this comment was not posted:
Confidence changes required:0%
The renaming of structs fromUploadItemtoPushItemandUploadToQueueRequesttoPushToQueueRequestis consistent with the new endpoint namepush_to_queue. This change is logical and aligns with the intent of the PR.
2. app-server/src/api/v1/queues.rs:40
- Draft comment:
RenamingUploadToQueueRequesttoPushToQueueRequestis consistent with the new endpoint namepush_to_queue. This aligns with the intent of the PR. - Reason this comment was not posted:
Confidence changes required:0%
The renaming of structs fromUploadItemtoPushItemandUploadToQueueRequesttoPushToQueueRequestis consistent with the new endpoint namepush_to_queue. This change is logical and aligns with the intent of the PR.
Workflow ID: wflow_6w20SKJRlCoUnp60
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c0984e4 in 1 minute and 17 seconds
More details
- Looked at
41lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_0hngwbrtckcHuPC3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Adds an API endpoint for uploading spans to a queue, updates server configuration, and modifies TypeScript route handling for optional fields.
push_to_queuefunction inqueues.rsto handle POST requests to/queues/push.PushItemandPushToQueueRequeststructs for deserializing request data.push_to_queueservice inmain.rsunder the/v1scope.remove/route.tsto handle optionalactionfields with null checks.This description was created by
for c0984e4. It will automatically update as commits are pushed.