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

[TSVB] Renamed 'positive rate' to 'counter rate' #80939

Merged
merged 3 commits into from
Oct 30, 2020
Merged

[TSVB] Renamed 'positive rate' to 'counter rate' #80939

merged 3 commits into from
Oct 30, 2020

Conversation

batchunag
Copy link
Contributor

@batchunag batchunag commented Oct 18, 2020 •
edited
Loading

Summary

Renamed the label and corresponding function/file names.
Todo: i18n translations for label.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@batchunag batchunag requested a review from a team October 18, 2020 02:44
@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?

@cla-checker-service
Copy link

cla-checker-service bot commented Oct 18, 2020 •
edited
Loading

💚 CLA has been signed

@batchunag batchunag marked this pull request as draft October 18, 2020 02:45
@batchunag
Copy link
Contributor Author

batchunag commented Oct 18, 2020 •
edited
Loading

Closes #80228

@timroes timroes added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Oct 19, 2020
@timroes
Copy link
Contributor

timroes commented Oct 19, 2020

Hi,

thanks a lot for this PR. Could you please make sure to sign the CLA above for us to be able to merge this code.

A couple of notes:

  • I think we should not rename the "id" of the aggregation as done in this PR. Otherwise you'll also need to provide a saved object migration, that will migrate previously created saved objects from the old id to the new one. I would just leave it as it is and just change the labels and therefore not requiring a saved object migration.
  • For translation of those labels (since I saw you left a TODO in your description): You only need to remove the labels you changed from all translation files, (e.g. remove the visTypeTimeseries.aggLookup.positiveRateLabel and other changed ids from the ja-JP.json and zh-CN.json file under x-pack/plugins/translations/translations). Our translation team will then take care of translating them for the new release again.

Cheers,
Tim

@batchunag batchunag marked this pull request as ready for review October 29, 2020 10:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@batchunag
Copy link
Contributor Author

@elasticmachine merge upstream

@batchunag
Copy link
Contributor Author

@timroes thanks for the feedback. I've updated the code.
Regarding to the CLA, I've already signed. Do I have to submit the pdf file to somewhere? I couldn't find information on that.

@timroes
Copy link
Contributor

timroes commented Oct 29, 2020

@batchunag I see that you signed the CLA with a @gmail.com mail address, but you made this PR from a @indeed.com mail address. Could you please either change the author of the commits, or sign the CLA also for your @indeed.com mail address? Thank you

@batchunag
Copy link
Contributor Author

Oh I see. Thanks.

@batchunag batchunag force-pushed the master branch 2 times, most recently from a747cd8 to 53bf7a6 Compare October 29, 2020 10:51
@batchunag
Copy link
Contributor Author

@elasticmachine merge upstream

@batchunag
Copy link
Contributor Author

@timroes CLA works now.

@stratoula
Copy link
Contributor

stratoula commented Oct 29, 2020 •
edited
Loading

@batchunag thank you a lot for this PR. I haven't made a thorough code review but I have two comments so far:

  • As Tim has already mentioned we would like to only change the labels of the positive rate to counter rate and not the type. Now when I try to select the Counter rate it is not selected and a wrong message is displayed:

Screenshot 2020-10-29 at 1 24 17 PM

that the `positive_rate` aggregation is not supported which is not the case here 😄
  • You have removed the translations from x-pack/plugins/translations/translations/ja-JP.json but you should also do it forx-pack/plugins/translations/translations/zh-CN.json

@batchunag
Copy link
Contributor Author

@stratoula thanks for the feedback. Looks like I'm making the code less readable. I was hesitant to leave some variable, method names half changed. Okay, I'll keep my changes only to the label text part.

@batchunag
Copy link
Contributor Author

@stratoula now we've much simpler update. Please have a look. I think the other labels such as visTypeTimeseries.positiveRate.helpText: "This aggregation should only be applied to {link}, it is a shortcut for applying max, derivative and counter only to a field." still valid regardless of the labels changed.

@stratoula stratoula added Feature:TSVB TSVB (Time Series Visual Builder) release_note:skip Skip the PR/issue when compiling release notes v7.11 v8.0.0 labels Oct 29, 2020
@stratoula
Copy link
Contributor

@elasticmachine merge upstream

@wylieconlon wylieconlon added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Oct 29, 2020
@wylieconlon wylieconlon changed the title Renamed 'positive rate' to 'counter rate' [TSVB] Renamed 'positive rate' to 'counter rate' Oct 29, 2020
@stratoula
Copy link
Contributor

Jenkins, test this.

@stratoula
Copy link
Contributor

@stratoula now we've much simpler update. Please have a look. I think the other labels such as visTypeTimeseries.positiveRate.helpText: "This aggregation should only be applied to {link}, it is a shortcut for applying max, derivative and counter only to a field." still valid regardless of the labels changed.

Thank you @batchunag! Yes, it is fine. Will check it asap 🙂

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
visTypeTimeseries 1.7MB 1.7MB -3.0B

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

stratoula
stratoula approved these changes Oct 30, 2020
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested it locally in Chrome and Safari. The positive rate has been renamed to counter rate everywhere. Thanx @batchunag for working on this 🍪

@stratoula stratoula merged commit 534f4e8 into elastic:master Oct 30, 2020
@stratoula stratoula mentioned this pull request Oct 30, 2020
Merged
stratoula pushed a commit to stratoula/kibana that referenced this pull request Oct 30, 2020
@batchunag @kibanamachine @stratoula
[TSVB] Renamed 'positive rate' to 'counter rate' (elastic#80939)
fc3f242 
* Renamed 'positive rate' label text to 'counter rate'

* Removed translations(ja-JP, zh-CN) where the labels were updated

Co-authored-by: Kibana Machine <[email protected]>
stratoula added a commit that referenced this pull request Oct 30, 2020
@stratoula @batchunag @kibanamachine
[TSVB] Renamed 'positive rate' to 'counter rate' (#80939) (#82102)
d20b820 
* Renamed 'positive rate' label text to 'counter rate'

* Removed translations(ja-JP, zh-CN) where the labels were updated

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: DB <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
@timroes timroes added v7.11.0 and removed v7.11 labels Oct 30, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 30, 2020
@gmmorris
Merge branch 'master' into alerts/convert-to-tm-intervals
1929b5d 
* master: (71 commits)
  [Chrome] Extension to append an element to the last breadcrumb (elastic#82015)
  [Monitoring] Thread pool rejections alert (elastic#79433)
  [Actions] Fix actionType type on registerType function (elastic#82125)
  [Security Solution] Modal for saving timeline (elastic#81802)
  add tests for index pattern switching (elastic#81987)
  TS project references for share plugin (elastic#82051)
  [Graph] Fix problem with duplicate ids (elastic#82109)
  skip 'returns a single bucket if array has 1'.  related elastic#81460
  Add a link to documentation in the alerts and actions management UI (elastic#81909)
  [Fleet] fix duplicate ingest pipeline refs (elastic#82078)
  Context menu trigger for URL Drilldown (elastic#81158)
  SO management: fix legacy import index pattern selection being reset when switching page (elastic#81621)
  Fixed dead links (elastic#78696)
  [Search] Add "restore" to session service (elastic#81924)
  fix Lens heading structure (elastic#81752)
  [ML] Data Frame Analytics: Fix feature importance cell value and decision path chart (elastic#82011)
  Remove legacy app arch items from codeowners. (elastic#82084)
  [TSVB] Renamed 'positive rate' to 'counter rate' (elastic#80939)
  Expressions/migrations2 (elastic#81281)
  [Telemetry] [Schema] remove number type and support all es number types (elastic#81774)
  ...
@timroes timroes mentioned this pull request Nov 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratoula stratoula stratoula approved these changes

Assignees

@stratoula stratoula

Labels
💝community Feature:TSVB TSVB (Time Series Visual Builder) release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@batchunag @kibanamachine @timroes @elasticmachine @stratoula @wylieconlon