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.

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

[APM] Agent span_frames_min_duration configuration input cannot handle 0 or -1 #62777

Merged
merged 2 commits into from
Apr 7, 2020

Conversation

cauemarcondes
Copy link
Contributor

closes #62569

@cauemarcondes cauemarcondes added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 apm:Agent Configuration labels Apr 7, 2020
@cauemarcondes cauemarcondes requested a review from a team as a code owner April 7, 2020 12:35
Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

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

It works in general, but if I set the value to -2 or less I get an error "Please specify an integer and a unit" on the field. This works in that we're validating that we don't want < -1, but is confusing. Could we put min="-1" on the number input element?

Let me know if that's not possible and I'll approve, but while this fixes the main problem, the < -1 behavior is still weird.

@cauemarcondes
Copy link
Contributor Author

It works in general, but if I set the value to -2 or less I get an error "Please specify an integer and a unit" on the field. This works in that we're validating that we don't want < -1, but is confusing. Could we put min="-1" on the number input element?

Let me know if that's not possible and I'll approve, but while this fixes the main problem, the < -1 behavior is still weird.

I can change the error message to something like Please specify an integer greater than -1 and a unit, WDYT?

@smith
Copy link
Contributor

smith commented Apr 7, 2020

@cauemarcondes

I can change the error message to something like Please specify an integer greater than -1 and a unit, WDYT?

Yes that's probably fine for now. Using the min attribute on the field is something we could come back to later.

@cauemarcondes
Copy link
Contributor Author

@cauemarcondes

I can change the error message to something like Please specify an integer greater than -1 and a unit, WDYT?

Yes that's probably fine for now. Using the min attribute on the field is something we could come back to later.

Ok, it was an easy fix to add the min property. 4321992, I added the default value as 1, because before the duration was validating if the value was > 0.

@cauemarcondes cauemarcondes requested a review from smith April 7, 2020 16:03
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@cauemarcondes cauemarcondes merged commit 20dc67d into elastic:master Apr 7, 2020
@cauemarcondes cauemarcondes deleted the agent-config-62569 branch April 7, 2020 18:18
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request Apr 7, 2020
…e 0 or -1 (elastic#62777)

* changing duration min value for span_frames_min_duration

* adding min property to number field
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request Apr 7, 2020
…e 0 or -1 (elastic#62777)

* changing duration min value for span_frames_min_duration

* adding min property to number field
cauemarcondes added a commit that referenced this pull request Apr 7, 2020
…e 0 or -1 (#62777) (#62844)

* changing duration min value for span_frames_min_duration

* adding min property to number field
cauemarcondes added a commit that referenced this pull request Apr 7, 2020
…e 0 or -1 (#62777) (#62845)

* changing duration min value for span_frames_min_duration

* adding min property to number field
@cauemarcondes cauemarcondes added the apm:test-plan-done Pull request that was successfully tested during the test plan label Apr 16, 2020
@cauemarcondes cauemarcondes self-assigned this Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:Agent Configuration apm:test-plan-done Pull request that was successfully tested during the test plan release_note:skip Skip the PR/issue when compiling release notes v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Agent span_frames_min_duration configuration input cannot handle 0 or -1
3 participants