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.

Sign up for GitHub

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

Jump to bottom

chore: rename functions in aggregated_transactions helper #116001

Merged
merged 9 commits into from
Oct 26, 2021
Merged

chore: rename functions in aggregated_transactions helper #116001

merged 9 commits into from
Oct 26, 2021

Conversation

deepak-sreekumar
Copy link
Contributor

Summary

Some functions in the helper include ForAggregatedTransactions in the name but they are meant to be used for both single transactions and aggregated transactions. Removed the term aggregated from such methods and moved to another helper.

Closes #114458

For maintainers

@deepak-sreekumar deepak-sreekumar requested a review from a team as a code owner October 21, 2021 18:38
@kibanamachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Oct 21, 2021
@cla-checker-service
Copy link

cla-checker-service bot commented Oct 21, 2021 •
edited
Loading

💚 CLA has been signed

@elasticmachine
Copy link
Contributor

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

@deepak-sreekumar deepak-sreekumar changed the title refactor(NA): rename functions in aggregated_transactions helper chore: rename functions in aggregated_transactions helper #release_note:skip Oct 21, 2021
@deepak-sreekumar deepak-sreekumar mentioned this pull request Oct 21, 2021
Closed
@deepak-sreekumar
Copy link
Contributor Author

I tried adding the release_note:skip label. But it looks like I don't have permission.
PS: I have signed CLA after raising the PR

@deepak-sreekumar deepak-sreekumar changed the title chore: rename functions in aggregated_transactions helper #release_note:skip chore: rename functions in aggregated_transactions helper Oct 21, 2021
@deepak-sreekumar
Copy link
Contributor Author

I have signed the CLA. Not sure why the status is still not reflected here. Am I missing something? @sqren

@dgieselaar
Copy link
Member

@elasticmachine merge upstream

@dgieselaar dgieselaar 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 Oct 22, 2021
@dgieselaar
Copy link
Member

@deepak-sreekumar looks like your commits come from two separate email addresses. You'll have to sign the CLA with both, or, reset the author for the commits from one of the email addresses.

@deepak-sreekumar
Copy link
Contributor Author

@dgieselaar Got it. Fixed the author for the commit. Thanks a lot!

@deepak-sreekumar
Copy link
Contributor Author

Status of kibana-ci seems to be stuck WIP. Will it only be triggered after review?

@sorenlouv sorenlouv requested review from sorenlouv and removed request for sorenlouv October 23, 2021 21:13
@sorenlouv sorenlouv added v8.0.0 and removed v7.16.0 labels Oct 23, 2021
@sorenlouv
Copy link
Member

@elasticmachine merge upstream

@sorenlouv
Copy link
Member

Buildkite, test this

@sorenlouv
Copy link
Member

You might need to run node scripts/eslint.js --fix

@deepak-sreekumar
Copy link
Contributor Author

@sqren Thanks! Fixed it. Please trigger the build again.

@sorenlouv
Copy link
Member

Buildkite, test this

@sorenlouv
Copy link
Member

@dgieselaar I changed the labels so this will not be backported to 7.16 since it's over FF and it's not a bug fix. Sounds ok or are you worried this will cause too many conflicts?

@sorenlouv
Copy link
Member

@elasticmachine run elasticsearch-ci/docs

sorenlouv
sorenlouv reviewed Oct 25, 2021
View reviewed changes
x-pack/plugins/apm/server/lib/helpers/aggregated_transactions/index.ts Outdated
return searchAggregatedTransactions
? ProcessorEvent.metric
: ProcessorEvent.transaction;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to rename helpers/aggregated_transactions/index.ts to helpers/transactions/index.ts instead of splitting it up (moving some functions and not others)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Will make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sqren Pushed the changes. Please trigger the build again

sorenlouv
sorenlouv requested changes Oct 25, 2021
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

Just a single change request

@dgieselaar
Copy link
Member

@sqren:

@dgieselaar I changed the labels so this will not be backported to 7.16 since it's over FF and it's not a bug fix. Sounds ok or are you worried this will cause too many conflicts?

sgtm!

@sorenlouv
Copy link
Member

Buildkite, test this

@deepak-sreekumar
Copy link
Contributor Author

Test Failures

  • [job] [logs] Jest Tests / timing validation should validate that min_age is equal or greater than previous phase min_age

Does Not seem to be related to the PR at first glance. Is this a flaky test? @sqren Please advise.

@sorenlouv
Copy link
Member

Buildkite, test this

@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #12 / detection engine api security and spaces enabled Detection exceptions data types and operators Rule exception operators for data type ip "is not in list" operator will return 3 results if we have a list which contains the CIDR ranges of "127.0.0.1/32, 127.0.0.2/31, 127.0.0.4/30"

Metrics [docs]

✅ unchanged

History

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

@sorenlouv
Copy link
Member

@elasticmachine run elasticsearch-ci/docs

@deepak-sreekumar
Copy link
Contributor Author

@sqren Hope everything is in order. Please let me know if there is anything else pending before the merge. Thanks a lot for the help!

@sorenlouv sorenlouv merged commit 2c5ecd9 into elastic:master Oct 26, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 116001

@sorenlouv sorenlouv added the backport:skip This commit does not require backporting label Oct 26, 2021
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 26, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into reporting/poc…
446dc6e 
…-migrate-away-from-injected-css-js

* 'master' of github.com:elastic/kibana: (347 commits)
  [Upgrade Assistant] Disable UI by default in 8.0 (elastic#115782)
  [Uptime] Added permission for new tls alert (elastic#116107)
  [APM] Optimize synthtrace (elastic#116091)
  Fix ux/apm inspector panel (elastic#116188)
  [RAC]: add experimental badge to alerts (elastic#116090)
  Unskip jest handled promise rejections (elastic#116021)
  [Lens] Improve tick placement for binary formatter (elastic#116158)
  chore: rename getApmHref to getLegacyApmHref (elastic#115689)
  [Security Solution] Validate ipv4/CIDR with format x.x.x.x/xx (elastic#116127)
  [Fleet] Use data stream name in query to get data stream info (elastic#115805)
  [Uptime] TLS and TLS legacy alert translation mismatch (elastic#116113)
  New field for integrations field (elastic#116175)
  Set required to false until the input is not visited (elastic#116099)
  Enable interactive setup by default (elastic#116141)
  Add not ready response to interactive setup (elastic#116138)
  Hide or button if needed (elastic#116124)
  [ML] Adding datafeed api tests (elastic#116133)
  Add page title to index advanced page (elastic#116134)
  chore: rename functions in aggregated_transactions helper  (elastic#116001)
  Fix bug where number rendered as date (elastic#116224)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/lib/screenshots/observable.ts
#	x-pack/plugins/reporting/server/lib/screenshots/open_url.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed backport:skip This commit does not require backporting 💝community needs_reviewer release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename functions in aggregated_transactions helper
5 participants
@deepak-sreekumar @kibanamachine @elasticmachine @dgieselaar @sorenlouv