[APM] Remove start and end from setupRequest #112828

MiriamAparicio · 2021-09-22T13:31:16Z

Summary

start and end are the only url params that not explicitly passed to the underlying handler.

What was done

Removed start and end from setupRequest
Get start and end params from the query
Fixed tests and types

sorenlouv · 2021-09-22T22:52:27Z

x-pack/plugins/apm/server/lib/alerts/chart_preview/get_transaction_error_rate.ts

+  const { apmEventClient } = setup;
+  const { serviceName, environment, transactionType, interval, start, end } =
+    alertParams;
+


minor thing but I noticed this elsewhere also: probably unintentional on your part but it's easier for a reviewer to understand the actual changes if lines are not re-ordered like here.

x-pack/plugins/apm/server/lib/transactions/get_transaction/index.ts

sorenlouv · 2021-09-22T22:57:57Z

x-pack/plugins/apm/server/lib/helpers/calculate_throughput.ts

+  start: number;
+  end: number;
+  value: number;


so much easier to read now 👍

x-pack/plugins/apm/server/lib/helpers/setup_request.ts

sorenlouv

Looks great! Thanks for doing this!

sorenlouv

Looks good to me. Can be merged when the comments in get_transaction/index.ts are resolved.

elasticmachine · 2021-09-27T08:34:39Z

Pinging @elastic/apm-ui (Team:apm)

elasticmachine · 2021-09-27T08:34:39Z

Pinging @elastic/uptime (Team:uptime)

justinkambic

UX changes LGTM. Smoke-tested on Edge as well.

kibanamachine · 2021-09-27T16:00:35Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`apm`	30	31	+1

History

💔 Build #155724 failed 8b8e986fac32d1dfa1767cb43d87876f1ead01e1
💔 Build #155663 failed 3910c6bd6207e65e91b523dbf2c5fc8d5a7623fd
💔 Build #155628 failed c93229d1084723d5e504204bd352493d580fb23c
💔 Build #155037 failed af57c6e69de1a31de180b248bd8759b417680aab
💔 Build #154985 failed 087ee96a1d6493dd0877c38011ed87dec12bc64c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

sorenlouv · 2021-09-27T23:23:17Z

x-pack/plugins/apm/server/lib/search_strategies/queries/get_query_with_params.ts

@@ -43,7 +43,7 @@ export const getQueryWithParams = ({ params, termFilters }: QueryParams) => {
    getOrElse<t.Errors, { start: number; end: number }>((errors) => {
      throw new Error(failure(errors).join('\n'));
    })
-  ) as Setup & SetupTimeRange;
+  ) as Setup & { start: number; end: number };


This looks a bit odd. It looks like we are creating a fake setup object that actually only contains start and end.

You should drop the explicit type annotations:

// converts string based start/end to epochmillis const decodedRange = pipe( rangeRt.decode({ start, end }), getOrElse<t.Errors, { start: number; end: number }>((errors) => { throw new Error(failure(errors).join('\n')); }) );

Sorry, I saw the comments after merging. I can do a small PR to fix this 🙏

* [APM] Remove start and end from setupRequest * fix some types * fix some conflicts * PR review comments * fix typing

kibanamachine · 2021-09-28T06:53:51Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* [APM] Remove start and end from setupRequest * fix some types * fix some conflicts * PR review comments * fix typing Co-authored-by: Miriam <31922082+MiriamAparicio@users.noreply.github.com>

MiriamAparicio requested a review from sorenlouv September 22, 2021 13:31

MiriamAparicio force-pushed the remove-start-end-params-from-setuprequest branch 3 times, most recently from 087ee96 to af57c6e Compare September 22, 2021 16:24

sorenlouv reviewed Sep 22, 2021

View reviewed changes

x-pack/plugins/apm/server/lib/transactions/get_transaction/index.ts Outdated Show resolved Hide resolved

sorenlouv reviewed Sep 22, 2021

View reviewed changes

x-pack/plugins/apm/server/lib/helpers/setup_request.ts Show resolved Hide resolved

sorenlouv approved these changes Sep 22, 2021

View reviewed changes

MiriamAparicio force-pushed the remove-start-end-params-from-setuprequest branch from af57c6e to c93229d Compare September 27, 2021 06:44

sorenlouv requested changes Sep 27, 2021

View reviewed changes

MiriamAparicio marked this pull request as ready for review September 27, 2021 08:16

MiriamAparicio requested review from a team as code owners September 27, 2021 08:16

MiriamAparicio added release_note:skip Skip the PR/issue when compiling release notes v7.16.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Sep 27, 2021

botelastic bot added Team:APM - DEPRECATED Use Team:obs-ux-infra_services. Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Sep 27, 2021

sorenlouv approved these changes Sep 27, 2021

View reviewed changes

MiriamAparicio added 5 commits September 27, 2021 14:55

[APM] Remove start and end from setupRequest

213a986

fix some types

9d0074b

fix some conflicts

96461f6

PR review comments

de9ded8

fix typing

68d8065

MiriamAparicio force-pushed the remove-start-end-params-from-setuprequest branch from 8b8e986 to 68d8065 Compare September 27, 2021 13:55

justinkambic approved these changes Sep 27, 2021

View reviewed changes

sorenlouv reviewed Sep 27, 2021

View reviewed changes

MiriamAparicio merged commit 7727bf4 into elastic:master Sep 28, 2021

MiriamAparicio deleted the remove-start-end-params-from-setuprequest branch September 28, 2021 06:51

kibanamachine mentioned this pull request Sep 28, 2021

[7.x] [APM] Remove start and end from setupRequest (#112828) #113220

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 28, 2021

[APM] Remove start and end from setupRequest (elastic#112828)

e55735f

* [APM] Remove start and end from setupRequest * fix some types * fix some conflicts * PR review comments * fix typing

MiriamAparicio mentioned this pull request Sep 28, 2021

Drop explicit type annotations, fix typing #113230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Remove start and end from setupRequest #112828

[APM] Remove start and end from setupRequest #112828

MiriamAparicio commented Sep 22, 2021

sorenlouv Sep 22, 2021

sorenlouv Sep 22, 2021

sorenlouv left a comment

sorenlouv left a comment

elasticmachine commented Sep 27, 2021

elasticmachine commented Sep 27, 2021

justinkambic left a comment

kibanamachine commented Sep 27, 2021

sorenlouv Sep 27, 2021

sorenlouv Sep 27, 2021 •

edited

Loading

MiriamAparicio Sep 28, 2021

sorenlouv Sep 28, 2021

kibanamachine commented Sep 28, 2021

[APM] Remove start and end from setupRequest #112828

[APM] Remove start and end from setupRequest #112828

Conversation

MiriamAparicio commented Sep 22, 2021

Summary

What was done

sorenlouv Sep 22, 2021

Choose a reason for hiding this comment

sorenlouv Sep 22, 2021

Choose a reason for hiding this comment

sorenlouv left a comment

Choose a reason for hiding this comment

sorenlouv left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 27, 2021

elasticmachine commented Sep 27, 2021

justinkambic left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 27, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing exports

History

sorenlouv Sep 27, 2021

Choose a reason for hiding this comment

sorenlouv Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

MiriamAparicio Sep 28, 2021

Choose a reason for hiding this comment

sorenlouv Sep 28, 2021

Choose a reason for hiding this comment

kibanamachine commented Sep 28, 2021

💚 Backport successful

sorenlouv Sep 27, 2021 •

edited

Loading