Skip to content
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

Batch Download of traces #1274

Merged
merged 7 commits into from
Mar 22, 2023

Conversation

Katarzyna-B
Copy link
Contributor

Batch Download of traces

Which problem is this PR solving?

Short description of the changes

  • Add a button with download functionality

@yurishkuro
Copy link
Member

Thanks for the PR. I have a couple of comments, but I posted them in the ticket to solicit opinions from people who voted for this feature: #663 (comment)

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make sure all commits are signed (see CONTRIBUTING.md)

</Button>
);
return (
<>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does <> represent? Is it needed?

Copy link
Contributor Author

@Katarzyna-B Katarzyna-B Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<> </>
This is called the Empty tag in React. This is a shorter syntax for React Fragments.
Documentation: https://react.dev/reference/react/Fragment
It is needed. If you remove this tag you will get exception like

[AltViewOptions.tsx](react-dom.development.js:14887 Uncaught Error: Objects are not valid as a React child (found: object with keys {downloadBtn})).....

It is used already in this project. See for example AltViewOptions.tsx

Copy link
Contributor Author

@Katarzyna-B Katarzyna-B Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong. I got the exception which I described above, but the lint complained about this part of code:

.../SearchResults/DownloadResults.tsx
[eslint ] 29:10 error Fragments should contain more than one child - otherwise, there’s no need for a Fragment at all react/jsx-no-useless-fragment

I changed the code in a different way and it works locally without the above described error.

document.body.appendChild(element);
element.click();
URL.revokeObjectURL(element.href)
element.remove()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading this as not a web developer, is this the preferred way of executing downloads from a page? Do you have some pointer to documentation describing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not primarily a web developer. But when I was looking for the best way of executing downloads from a page I found bunch of similar solution how you can do it. Here some references:

element.click() --> it's simulate a click
URL.revokeObjectURL(element.href) --> "it is recommended to release object URLs to optimize performance and reduce memory consumption." In the first article it is good explained.

@@ -114,7 +115,7 @@ function searchDone(state, { meta, payload }) {
}
const traces = { ...state.traces, ...resultTraces };
const search = { ...state.search, results, state: fetchedState.DONE };
return { ...state, search, traces };
return { ...state, search, traces, tracesToDownload: payloadData};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't think this reducer knows why it's returning the full payload, so naming the output tracesToDownload is not accurate. Can we name it something like rawTraces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I will do it.

Katarzyna-B and others added 3 commits March 20, 2023 14:16
Signed-off-by: katarzyna <katarzyna@smaato.com>
## Which problem is this PR solving?
- Unbreak the UI in the production build (closes jaegertracing#1275)
## Short description of the changes
It seems that the antd v3 upgrade in
cda1746 causes an error in the
production build, apparently due to a bad interplay with vite/esbuild
bundling. As a workaround, provide an import alias for the antd icon
sprite file that's causing the issue.[1]

---
[1]
ant-design/ant-design#19002 (comment)

Signed-off-by: Máté Szabó <mszabo@fandom.com>
- Resolves jaegertracing#663

## Short description of the changes
- Add a button with download functionality

Signed-off-by: katarzyna <katarzyna@smaato.com>
Katarzyna-B and others added 2 commits March 20, 2023 16:08
- Resolves jaegertracing#663

## Short description of the changes
- Add a button with download functionality

Signed-off-by: katarzyna <katarzyna@smaato.com>
yurishkuro
yurishkuro previously approved these changes Mar 21, 2023
@yurishkuro
Copy link
Member

please fix lint errors

- Resolves jaegertracing#663

## Short description of the changes
- Add a button with download functionality

Signed-off-by: katarzyna <katarzyna@smaato.com>
- Resolves jaegertracing#663

## Short description of the changes
- Add a button with download functionality

Signed-off-by: katarzyna <katarzyna@smaato.com>
@Katarzyna-B Katarzyna-B requested review from yurishkuro and removed request for tiffon, everett980 and rubenvp8510 March 21, 2023 22:18
@yurishkuro yurishkuro merged commit c1511b7 into jaegertracing:main Mar 22, 2023
Binrix pushed a commit to Binrix/jaeger-ui that referenced this pull request Apr 18, 2023
Batch Download of traces
## Which problem is this PR solving?
- Resolves jaegertracing#663

## Short description of the changes
- Add a button with download functionality

---------

Signed-off-by: katarzyna <katarzyna@smaato.com>
Signed-off-by: Máté Szabó <mszabo@fandom.com>
Co-authored-by: Máté Szabó <mszabo@fandom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch Download of traces
3 participants