Rendering: Adds PDF support behind feature toggle #81811
Conversation
AgnesToulet
added
area/image-rendering
no-backport
AgnesToulet
requested review from
Clarity-89,
eledobleefe,
wbrowne and
oshirohugo
and removed request for
a team
AgnesToulet
requested review from
xnyo,
rwwiv,
JacobsonMT,
yuri-tceretian,
grobinson-grafana,
mildwonkey,
suntala and
yangkb09
and removed request for
a team
| Encoding: queryReader.Get("encoding", ""), | ||
| Encoding: encoding, |
There was a problem hiding this comment.
@torkelo Maybe we should actually remove this as I don't think we are ready for supporting PDFs here (this won't be behind the feature flag, this makes a currently paid feature free, and I think it should have a UI to set PDF options like zoom and orientation).
There was a problem hiding this comment.
@AgnesToulet not sure it matters that much, makes the development of this feature so much easier / faster. PDF support in the image renderer will exposed anyway. This just handles the encoding param so that we return the right content type.
and I think it should have a UI to set PDF options like zoom and orientation).
Don't we have that already? (in enterprise)
I think it's fine to expose a param like this that is not documented / work in progress. As long as not exposed in the UI (without feature toggle) or documented as a supported api feature
There was a problem hiding this comment.
This is a different endpoint in Enterprise. Why is it better to support this in this endpoint rather than in the Enterprise one? (available from the Share > PDF modal)
There was a problem hiding this comment.
@AgnesToulet what endpoint is that? can it render any url as well?
There was a problem hiding this comment.
It's this one: https://github.com/grafana/grafana-enterprise/blob/main/src/pkg/extensions/report/api/api.go#L123
It's not working with any URL but this calls the same function as the reporting feature.
The issue with using this OSS endpoint to test things out is that you may not have the same result as the PDF feature and break things there while fixing things for this endpoint that we don't really want to support.
There was a problem hiding this comment.
I am sorry I don't understand what url / api to use to render dashboard pdf using the new d-report route,
It could be useful to expose while developing so you can directly pass different params to image-renderer service without a rebuild.
But if there is a simple simple api in enterprise then I can use that and we can revert this encoding change.
There was a problem hiding this comment.
I don't see any problem using the current enpoint -- it is new behavior with new parameter (at least setting pdf as encoding was nonsense before)
Honestly, I'm not convinced we nee the feature toggle newPDFRendering -- but that should buy us time to get the capabilities stuff sorted like: https://github.com/grafana/grafana/blob/v10.3.1/pkg/services/rendering/capabilities.go#L26
There was a problem hiding this comment.
@AgnesToulet yea, the enterprise feature not being able to capture any URL as a PDF (So many online/OSS services that can do that), it's the report definition, layout and display options, and scheduling and emailing etc.
There was a problem hiding this comment.
@torkelo You can use it like this: http://localhost:3000/api/reports/render/pdfs?orientation=landscape&layout=grid&scaleFactor=2&dashboards=[{"dashboard":{"uid":"foKOhxxVk"}}]. This is less flexible than the /render endpoint but this is what is called when you export a dashboard into PDF in Grafana.
@ryantxu The feature toggle is needed in Enterprise (see associated PR) so it's using this new workflow instead of the new one. We can't release it directly because, as Torkel said, the reporting feature has a lot more options that we don't support yet.
I guess we can keep this for quick testing, but we should not forget to test the actual endpoint at the end.
Also, for simple PDF export, we should also look into doing it in the frontend directly and not call the image renderer as it should not be needed anymore (but this is not our current priority; our current priority is to reach feature parity with the current PDF export feature).
* start pdf refactor * Update AppChrome.tsx * Update AppChrome.tsx * add encoding param to rendering grpc service * fix plugin mode * clean up * fix backend tests * fix lint errors * Support pdf encoding in render http api --------- Co-authored-by: Torkel Ödegaard <torkel@grafana.com>
What is this feature?
This PR adds PDF support in the rendering service behind the
newPDFRenderingfeature toggle.This will be used for the paid reporting feature.
Why do we need this feature?
This will improve the performance and the maintenance of the reporting feature.
Who is this feature for?
Reporting users.
Special notes for your reviewer:
Associated Enterprise PR: https://github.com/grafana/grafana-enterprise/pull/6232
Associated Image Renderer PR: grafana/grafana-image-renderer#487