-
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
[Reporting] APM integration for baseline performance measurements #59967
[Reporting] APM integration for baseline performance measurements #59967
Conversation
5fca169
to
1b2a777
Compare
const item = elementsPositionAndAttributes[i]; | ||
const base64EncodedData = await asyncDurationLogger( |
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.
asyncDurationLogger
was hiding a type error, because we're resolving a string, not a Buffer
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.
More context about this: since we'll have APM to measure the duration of all these modules, we no longer need this helper function. After having removed it, I noticed the surrounding code treated the helper function return value as a Buffer but it is actually a string of base64 content.
1b2a777
to
4a5af92
Compare
4a5af92
to
a865bcc
Compare
Pinging @elastic/kibana-reporting-services (Team:Reporting Services) |
x-pack/legacy/plugins/reporting/export_types/printable_pdf/server/lib/tracker.ts
Show resolved
Hide resolved
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.
Amazing job on doing the full custom instrumentation using the APM Node.js agent. Love to see we are dog fooding more.
I just have one small suggestion, It just personal opinion as Span can be null | Span at any point in time. You could have a generic tracer utility
// utility
function startTrace() {
const span = apm.startSpan(...);
return () {
span && span.end()
}
}
// in actual code
const endTrace = startTrace(name, type)
endTrace()
Helps to avoid the if
check everywhere.
x-pack/legacy/plugins/reporting/export_types/common/lib/screenshots/open_url.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/reporting/export_types/printable_pdf/server/lib/tracker.ts
Show resolved
Hide resolved
...ck/legacy/plugins/reporting/export_types/common/lib/screenshots/get_element_position_data.ts
Outdated
Show resolved
Hide resolved
@@ -54,13 +54,14 @@ export const executeJobFactory: QueuedPngExecutorFactory = async function execut | |||
job.layout | |||
); | |||
}), | |||
map(({ buffer, warnings }) => { | |||
map(({ base64, warnings }) => { |
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.
I had to rename this - it was very unclear that the return payload actually is a string
@elasticmachine merge upstream |
Will merge this after the 7.8 branch is cut |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…astic#59967) * apm stuff * fix cluster_client * fix snapshot * tracker utility for generate_pdf * call apm.startSpan instead of txn.startSpan * Fix async call to end transaction * fix typescript * remove captuureErrors * restore accidental removal * add startTrace lib * fix import * fix imports * ts fix * fix generate_png to not format base64 to buffer and back to base64 * 💅 * revert change to cluster client * fix unused translation Co-authored-by: Elastic Machine <[email protected]>
…or-part-mvp-2 * 'master' of github.com:elastic/kibana: (259 commits) SavedObjects bulkCreate API should return migrationVersion and strip the type & namespace from the id (elastic#65150) Drilldown count tooltip (elastic#65105) plugins logs start with "plugins." prefix (elastic#65710) [ML] Fix pagination reset on search query update. (elastic#65668) [SIEM] Add types to the mappings objects so extra keys cannot be introduced [apm] Update machine learning flyout and service maps docs (elastic#65517) change api endpoint and throw error (elastic#65790) [Maps] remove SLA percentage metric (elastic#65718) [Reporting] APM integration for baseline performance measurements (elastic#59967) fix(NA): noParse regex for windows on kbn optimizer (elastic#65755) [ML] DFA: ensure at least one field is included in analysis before job can be created (elastic#65320) [Data plugin] cleanup - remove unused getRoutes / routes from indexPattern object (elastic#65683) Removed skip to enable test. (elastic#65575) [Lens] Type safe migrations (elastic#65576) [Canvas] Fix nav link behavior in Canvas (elastic#65590) [Event log] Fix flaky test (elastic#65658) [Alerting] changes preconfigured actions config from array to object (elastic#65397) remove immediate functions from esqueue worker cycles (elastic#65375) [Metrics UI] Fix isAbove to only display when threshold set (elastic#65540) draft search profiler accessibility tests (elastic#62357) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx
…9967) (#65797) * apm stuff * fix cluster_client * fix snapshot * tracker utility for generate_pdf * call apm.startSpan instead of txn.startSpan * Fix async call to end transaction * fix typescript * remove captuureErrors * restore accidental removal * add startTrace lib * fix import * fix imports * ts fix * fix generate_png to not format base64 to buffer and back to base64 * 💅 * revert change to cluster client * fix unused translation Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
Part of #59396
Refer to #43548 for information about Kibana's APM integrations.
Adds APM transactions and spans around the slow parts of Reporting code.
This lets us see a trace of where the flow is taking the most time when screenshots are captured. Here is an example of capturing a Dashboard with "print_layout" mode. You can see the multiple calls to
get_screenshots
:Checklist
Delete any items that are not applicable to this PR.
[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] This was checked for keyboard-only and screenreader accessibility[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser[ ] This was checked for cross-browser compatibility, including a check against IE11For maintainers