-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Lens] Enable report sharing #153429
[Lens] Enable report sharing #153429
Changes from 16 commits
76b5ebc
7fbac81
7b49555
dc40345
f408271
ef9246e
9c21584
4d885eb
b9f06e7
b1e0f7c
f638c8f
7422a61
429d5f5
a359e4b
e556fe1
85a42f2
70c9ed8
2228506
22dfdb5
daad4c7
fa61289
2b3d9e9
d73df1e
a1b3006
5e711ca
5040e20
82c18f1
719afe2
39ca589
f7b8943
e8c4e5f
39dcb2e
23286a3
fe0d606
a414421
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,8 +45,7 @@ function getShareURLForSavedObject( | |
); | ||
} | ||
|
||
function getShortShareableURL( | ||
shortUrlService: (params: LensAppLocatorParams) => Promise<string>, | ||
export function getLocatorParams( | ||
data: LensAppServices['data'], | ||
{ | ||
filters, | ||
|
@@ -80,7 +79,7 @@ function getShortShareableURL( | |
const serializableDatasourceStates = datasourceStates as LensAppState['datasourceStates'] & | ||
SerializableRecord; | ||
|
||
return shortUrlService({ | ||
return { | ||
filters, | ||
query, | ||
resolvedDateRange: getResolvedDateRange(data.query.timefilter.timefilter), | ||
|
@@ -90,16 +89,19 @@ function getShortShareableURL( | |
searchSessionId: data.search.session.getSessionId(), | ||
references, | ||
dataViewSpecs: adHocDataViews, | ||
}); | ||
}; | ||
} | ||
|
||
export async function getShareURL( | ||
shortUrlService: (params: LensAppLocatorParams) => Promise<string>, | ||
services: Pick<LensAppServices, 'application' | 'data'>, | ||
configuration: ShareableConfiguration | ||
configuration: ShareableConfiguration, | ||
shareUrlEnabled: boolean | ||
) { | ||
const locatorParams = getLocatorParams(services.data, configuration); | ||
return { | ||
shareableUrl: await getShortShareableURL(shortUrlService, services.data, configuration), | ||
shareableUrl: await (shareUrlEnabled ? shortUrlService(locatorParams) : undefined), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm seeing this is the code behind the note in the description that a short URL is created every time that a report is requested, but I think we should avoid this. For Reporting purposes, the The dashboard app uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
URLs are not used for reporting in the V2 export types. Eventually, the code for V1 types will be removed as usage of those APIs goes down, and all available export types will use locators. I agree with you the fact that a shortURL being created is independent of the reporting integration. I also think the Sharing panel should not require a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in fa61289 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added also a functional test to verify the correctness of the URL generated by reporting in this case ( d73df1e ) |
||
savedObjectURL: getShareURLForSavedObject(services, configuration.currentDoc), | ||
locatorParams, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -624,6 +624,12 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ | |
); | ||
}); | ||
|
||
function dispatchRenderComplete(node: HTMLDivElement | null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's also a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @vadimkibana There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's good to use your inline dispatcher. |
||
if (node) { | ||
node.dispatchEvent(new CustomEvent('renderComplete', { bubbles: true })); | ||
} | ||
} | ||
|
||
export const VisualizationWrapper = ({ | ||
expression, | ||
framePublicAPI, | ||
|
@@ -654,6 +660,9 @@ export const VisualizationWrapper = ({ | |
onData$: (data: unknown, adapters?: Partial<DefaultInspectorAdapters>) => void; | ||
}) => { | ||
const context = useLensSelector(selectExecutionContext); | ||
// Used for reporting | ||
const [isLoading, setIsLoading] = useState(true); | ||
const nodeRef = useRef<HTMLDivElement>(null); | ||
const searchContext: ExecutionContextSearch = useMemo( | ||
() => ({ | ||
query: context.query, | ||
|
@@ -719,7 +728,13 @@ export const VisualizationWrapper = ({ | |
} | ||
|
||
return ( | ||
<div className="lnsExpressionRenderer"> | ||
<div | ||
className="lnsExpressionRenderer" | ||
data-shared-items-container | ||
data-render-complete={!isLoading} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is an error encountered during rendering, such as a search request timeout, you should use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 2b3d9e9 |
||
data-shared-item="" | ||
ref={nodeRef} | ||
> | ||
<ExpressionRendererComponent | ||
className="lnsExpressionRenderer__component" | ||
padding="m" | ||
|
@@ -729,7 +744,11 @@ export const VisualizationWrapper = ({ | |
onEvent={onEvent} | ||
hasCompatibleActions={hasCompatibleActions} | ||
onData$={onData$} | ||
onRender$={onRender$} | ||
onRender$={() => { | ||
setIsLoading(false); | ||
onRender$(); | ||
dispatchRenderComplete(nodeRef.current); | ||
}} | ||
inspectorAdapters={lensInspector.adapters} | ||
executionContext={executionContext} | ||
renderMode="edit" | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 works well for unsaved content in Lens, but when there is a saved Lens object loaded without any unsaved changes, the locator params should use the
savedObjectId
field of LensAppLocatorParams. This will enable a common use case, where users copy a POST URL from the UI, and use it as a template to programmatically generate reports. Having the ability to replace the saved object ID as-needed is indispensable for this use case.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.
Because of the way URLs are generate for saved visualizations and unsaved, the URL format are pretty different (explicit vs short URLs).
Due to this difference the
shareableUrlForSavedObject
property has been added and used also for the reporting when avaialble: https://github.com/elastic/kibana/pull/153429/files#diff-be49f342798d67aec36153234bac3806d1b22566c1cd71a84b95be7c77157cc4R121There 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'm sorry, I meant for my comment to be specific only to the
locatorParams
, and not the shareableUrl. The locator params are what actually gets used as the app state for the Reporting API.To clarify: Let's say I opened from the Visualize Library, and made no changes, and then click the "Copy POST URL" button. The result looks like:
http://localhost:5601/api/reporting/generate/pngV2?jobParams=%28browserTimezone%3AAmerica%2FPhoenix%2Clayout%3A%28dimensions%3A%28height%3A593%2Cwidth%3A1171.7421875%29%2Cid%3Apreserve_layout%29%2ClocatorParams%3A%28id%3ALENS_APP_LOCATOR%2Cparams%3A%28activeDatasourceId%3AformBased%2CdataViewSpecs%3A%21%28%29%2CdatasourceStates%3A%28formBased%3A%28isLoading%3A%21f%2Cstate%3A%28currentIndexPatternId%3Aff959d40-b880-11e8-a6d9-e546fe2bba5f%2Clayers%3A%28%2797c63ea6-9305-4755-97d1-0f26817c6f9a%27%3A%28columnOrder%3A%21%28%279f61a7df-198e-4754-b34c-81ed544136ba%27%2Cebcb19af-0900-4439-949f-d8cd9bccde19%2C%275575214b-7f21-4b6c-8bc1-34433c6a0c58%27%29%2Ccolumns%3A%28%275575214b-7f21-4b6c-8bc1-34433c6a0c58%27%3A%28dataType%3Anumber%2CisBucketed%3A%21f%2Clabel%3A%27Count%20of%20records%27%2CoperationType%3Acount%2Cscale%3Aratio%2CsourceField%3A___records___%29%2C%279f61a7df-198e-4754-b34c-81ed544136ba%27%3A%28dataType%3Astring%2CisBucketed%3A%21t%2Clabel%3A%27Top%20values%20of%20category.keyword%27%2CoperationType%3Aterms%2Cparams%3A%28missingBucket%3A%21f%2CorderBy%3A%28columnId%3A%275575214b-7f21-4b6c-8bc1-34433c6a0c58%27%2Ctype%3Acolumn%29%2CorderDirection%3Adesc%2CotherBucket%3A%21t%2CparentFormat%3A%28id%3Aterms%29%2Csize%3A10%29%2Cscale%3Aordinal%2CsourceField%3Acategory.keyword%29%2Cebcb19af-0900-4439-949f-d8cd9bccde19%3A%28dataType%3Adate%2CisBucketed%3A%21t%2Clabel%3Aorder_date%2CoperationType%3Adate_histogram%2Cparams%3A%28includeEmptyRows%3A%21t%2Cinterval%3A%271d%27%29%2Cscale%3Ainterval%2CsourceField%3Aorder_date%29%29%2CincompleteColumns%3A%28%29%2CindexPatternId%3Aff959d40-b880-11e8-a6d9-e546fe2bba5f%29%29%29%29%29%2Cfilters%3A%21%28%29%2Cquery%3A%28language%3Akuery%2Cquery%3A%27%27%29%2Creferences%3A%21%28%28id%3Aff959d40-b880-11e8-a6d9-e546fe2bba5f%2Cname%3Aindexpattern-datasource-layer-97c63ea6-9305-4755-97d1-0f26817c6f9a%2Ctype%3Aindex-pattern%29%29%2CresolvedDateRange%3A%28fromDate%3A%272023-03-21T19%3A16%3A12.417Z%27%2CtoDate%3A%272023-03-28T19%3A16%3A12.417Z%27%29%2CsearchSessionId%3A%2743960930-60e1-42e3-a611-c6ae4a3af63c%27%2Cvisualization%3A%28activeId%3AlnsXY%2Cstate%3A%28axisTitlesVisibilitySettings%3A%28x%3A%21f%2CyLeft%3A%21f%2CyRight%3A%21t%29%2CfittingFunction%3ANone%2CgridlinesVisibilitySettings%3A%28x%3A%21t%2CyLeft%3A%21t%2CyRight%3A%21t%29%2Clayers%3A%21%28%28accessors%3A%21%28%275575214b-7f21-4b6c-8bc1-34433c6a0c58%27%29%2ClayerId%3A%2797c63ea6-9305-4755-97d1-0f26817c6f9a%27%2ClayerType%3Adata%2Cposition%3Atop%2CseriesType%3Abar_percentage_stacked%2CshowGridlines%3A%21f%2CsplitAccessor%3A%279f61a7df-198e-4754-b34c-81ed544136ba%27%2CxAccessor%3Aebcb19af-0900-4439-949f-d8cd9bccde19%29%29%2Clegend%3A%28isVisible%3A%21t%2ClegendSize%3Aauto%2Cposition%3Aright%29%2CpreferredSeriesType%3Abar_percentage_stacked%2CtickLabelsVisibilitySettings%3A%28x%3A%21t%2CyLeft%3A%21t%2CyRight%3A%21t%29%2CvalueLabels%3Ashow%2CyLeftExtent%3A%28mode%3Afull%29%2CyRightExtent%3A%28mode%3Afull%29%29%29%29%29%2CobjectType%3Alens%2Ctitle%3A%27Report%20Test%27%2Cversion%3A%278.8.0%27%29
After decoding the query string contents and parsing the rison into JSON, the locator params come out to:
For a saved Lens with no changes, we need a more optimized form of
locatorParams
, since they go into a URL that is often handled outside of Kibana. The data in the locatorParams should be simplified and reflect the saved object ID backing the Lens, plus the global time range since that isn't part of the saved object:This shortens the URL which is great for several reasons:
For all these reasons, we should avoid having the locatorParams contain just the full internal state of what is stored in the saved object. We should make it as referential as it can be, by using the
savedObjectId
field of theLensAppLocatorParams
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.
Fixed in fa61289