Skip to content

Conversation

@bartvanremortele
Copy link
Contributor

@bartvanremortele bartvanremortele commented Jul 21, 2019

Summary

Fixes #41566
Adds time formatting support for high duration transactions.
Formatting will be moved up to minutes if the transaction took longer than 1 minute and to hours if it took longer than 1 hour.

i18n for JPN was done using google, I do not speak Japanese.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@bartvanremortele bartvanremortele requested a review from a team as a code owner July 21, 2019 15:00
@elasticmachine
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?

@bartvanremortele bartvanremortele changed the title [APM] Add support for very high durations (minutes and hours) #41566 [APM] Add support for very high durations (minutes and hours) Jul 21, 2019
@sorenlouv
Copy link
Member

retest

@sorenlouv
Copy link
Member

Hi @bartvanremortele,
Thanks for your contribution. Overall it looks great 👍

Copy link
Member

Choose a reason for hiding this comment

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

The transaction files are auto-generated so if you created this and the Japanese by hand you can just discard them again.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sorenlouv
Copy link
Member

@bartvanremortele Whenever you have time please rebase this with master so I can merge. Thanks again for the contribution!

@bartvanremortele
Copy link
Contributor Author

@sqren Done! I would love to contribute more but it's hard to identify which issues I'm able to pick up. Do you have a mentorship program or a platform to onboard contributers?

@bartvanremortele
Copy link
Contributor Author

Hmm I ran node scripts/update_prs 41640 assuming it would rebase but it merged master into the PR branch.

@bartvanremortele bartvanremortele force-pushed the enhancement/high-duration-support branch 2 times, most recently from 6f90107 to ad2c276 Compare July 25, 2019 15:33
@bartvanremortele
Copy link
Contributor Author

Fixed! :-)

@sorenlouv
Copy link
Member

@katrin-freihofner @dgieselaar Do you have any opinions on whether the display value for hours should be "hours", "hrs" or "h". Same with minutes. Currently we abbreviate seconds to "s", millis to "ms" and micros to "μs".

Hours
Screen Shot 2019-07-25 at 18 07 18

Minutes
Screen Shot 2019-07-25 at 18 06 55

@sorenlouv
Copy link
Member

sorenlouv commented Jul 25, 2019

@bartvanremortele I'll just get my colleagues opinion on the unit abbreviation, and then I'll merge it.

Done! I would love to contribute more but it's hard to identify which issues I'm able to pick up. Do you have a mentorship program or a platform to onboard contributers?

We try to apply the label "good first issue" to make it easier for the community to help out - but you probably know that since you found this one :)
We don't have a mentorship program AFAIK but I'll bring it up in the next team meeting (next week) and see what we can do.

@dgieselaar
Copy link
Member

dgieselaar commented Jul 25, 2019 via email

@katrin-freihofner
Copy link
Contributor

I would suggest to use
ms...milliseconds
s...seconds
min...minutes
h...hours

but as long as it is not m (like meters) for minutes I'm fine :) Thanks everyone!

@katrin-freihofner
Copy link
Contributor

...sorry, closing this was a mistake.

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

Choose a reason for hiding this comment

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

As suggested by @katrin-freihofner minutes should be abbreviated "min"

@sorenlouv
Copy link
Member

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv force-pushed the enhancement/high-duration-support branch from 56ec952 to 143cd68 Compare July 26, 2019 10:57
@sorenlouv
Copy link
Member

sorenlouv commented Jul 26, 2019

@bartvanremortele I rebased your branch with master - hopefully that'll fix the build errors.

@sorenlouv
Copy link
Member

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sorenlouv sorenlouv merged commit 7ff85c4 into elastic:master Jul 26, 2019
@sorenlouv
Copy link
Member

sorenlouv commented Jul 26, 2019

@bartvanremortele This needs to be backported to "7.x" which is the release branch for 7.4, 7.5 and all other future releases on 7.

I can do it for you but if you are interested in doing it yourself I can help out. Basically you just need to cherry-pick 7ff85c4 from master and create a PR towards 7.x branch.

This is boring work, so we use a backport tool. If you already have cloned kibana you don't need to install the backport tool again, but can simply run yarn backport (inside the kibana folder). It will probably complain about missing github credentials in {homefolder}/.backport/config.json.

Follow these steps to fill out the config.json: https://github.com/sqren/backport/blob/master/docs/configuration.md#global-config-backportconfigjson

@bartvanremortele
Copy link
Contributor Author

@sqren I will gladly do this. Thanks for giving me the opportunity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[APM] Add support for very high durations (minutes and hours)

5 participants