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

Fix validation for duration vars on APM configuration settings #124336

Merged
merged 2 commits into from
Feb 3, 2022
Merged

Fix validation for duration vars on APM configuration settings #124336

merged 2 commits into from
Feb 3, 2022

Conversation

MiriamAparicio
Copy link
Contributor

Closes #121640

Screen.Recording.2022-02-02.at.11.26.25.mov

@MiriamAparicio MiriamAparicio added bug Fixes for quality problems that affect the customer experience Team:APM All issues that need APM UI Team support v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 v8.0.1 labels Feb 2, 2022
@MiriamAparicio MiriamAparicio requested review from a team as code owners February 2, 2022 11:54
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Feb 2, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@MiriamAparicio MiriamAparicio added the release_note:skip Skip the PR/issue when compiling release notes label Feb 2, 2022
juliaElastic
juliaElastic reviewed Feb 2, 2022
View reviewed changes
...ications/fleet/sections/agent_policy/edit_package_policy_page/utils/fix_apm_duration_vars.ts Outdated
import { DURATION_APM_SETTINGS_VARS } from '../../../../constants';

// Fix duration vars, if it's a migrated setting, and it's a plain old number with no suffix, we should assume seconds
export function fixApmDurationVars(vars: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could the type be more specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi, the type for newVars is any where we call the function, I can try to create a more specific type but not sure if it will work with other vars not APM specific

image

Copy link
Contributor

@juliaElastic juliaElastic Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

you could try with this:

kibana/x-pack/plugins/fleet/common/types/models/package_policy.ts

Line 48 in 2781a85

vars?: PackagePolicyConfigRecord;

or you could create a meaningful type for the fixApmDurationVars method and cast it at the call site

juliaElastic
juliaElastic reviewed Feb 2, 2022
View reviewed changes
...ins/fleet/public/applications/fleet/sections/agent_policy/edit_package_policy_page/index.tsx Outdated
@@ -232,6 +230,8 @@ export const EditPackagePolicyForm = memo<{
...basePolicyInputVars,
};
}
// Fix duration vars, if it's a migrated setting, and it's a plain old number with no suffix
newVars = fixApmDurationVars(newVars);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe there could be a check to only run this logic for apm package type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where I can check if it's an apm package type?

Copy link
Contributor

Choose a reason for hiding this comment

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

here you can use basePackage: basePackage.name === 'apm'

juliaElastic
juliaElastic reviewed Feb 2, 2022
View reviewed changes
...ications/fleet/sections/agent_policy/edit_package_policy_page/utils/fix_apm_duration_vars.ts
DURATION_APM_SETTINGS_VARS;
// convert vars to array, map each key/value pair into another pair
// and then fromEntries gives back the object
return Object.fromEntries(
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add unit tests for this logic?

@juliaElastic
Copy link
Contributor

I'm wondering if these vars could be fixed on the apm package definition itself, it doesn't look good to call a package specific logic in fleet code
cc @joshdover

@MiriamAparicio MiriamAparicio force-pushed the fix-validation-warning-apm-integration-editor branch from e6a40f1 to cd1eaeb Compare February 2, 2022 14:59
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #21 / transform creation with runtime mappings batch transform with unique rt_airline_lower and sort by time and runtime mappings navigates through the wizard and sets all needed fields

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fleet 565 566 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 2.8MB 2.8MB +16.0B
fleet 659.0KB 659.3KB +372.0B
total +388.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 109.2KB 109.4KB +208.0B

History

  • 💔 Build #21316 failed e6a40f13fb379e60e7cbab748266c5459dee65d2

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

ogupte
ogupte approved these changes Feb 2, 2022
View reviewed changes
Copy link
Contributor

@ogupte ogupte left a comment

Choose a reason for hiding this comment

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

As a bug fix, this works great, but eventually we'll want to consolidate these var types with the other var for fleet migration #123483. Well done!

@juliaElastic
Copy link
Contributor

is the flaky test related in any way to this change?

juliaElastic
juliaElastic approved these changes Feb 3, 2022
View reviewed changes
Copy link
Contributor

@juliaElastic juliaElastic left a comment

Choose a reason for hiding this comment

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

LGTM

@MiriamAparicio MiriamAparicio merged commit c51daa7 into elastic:main Feb 3, 2022
@MiriamAparicio MiriamAparicio deleted the fix-validation-warning-apm-integration-editor branch February 3, 2022 11:57
@kibanamachine
Copy link
Contributor

The following labels were identified as gaps in your version labels and will be added automatically:

  • v8.2.0

If any of these should not be on your pull request, please manually remove them.

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 3, 2022
@MiriamAparicio @kibanamachine
Fix validation for duration vars on APM configuration settings (elast…
a2bc10e 
…ic#124336)

* Fix validation for duration vars on apm configuration settings

* Add unit test and just do the fix if the base package is apm

(cherry picked from commit c51daa7)
@kibanamachine kibanamachine mentioned this pull request Feb 3, 2022
Merged
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
❌ 8.0 Backport failed because of merge conflicts
✅ 8.1

How to fix

Re-run the backport manually:

node scripts/backport --pr 124336

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@MiriamAparicio MiriamAparicio mentioned this pull request Feb 4, 2022
Closed
MiriamAparicio added a commit to MiriamAparicio/kibana that referenced this pull request Feb 4, 2022
@MiriamAparicio
Fix validation for duration vars on APM configuration settings (elast…
d8d1582 
…ic#124336)

* Fix validation for duration vars on apm configuration settings

* Add unit test and just do the fix if the base package is apm

(cherry picked from commit c51daa7)

# Conflicts:
#	x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/edit_package_policy_page/index.tsx
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 4, 2022
kibanamachine added a commit that referenced this pull request Feb 4, 2022
@kibanamachine @MiriamAparicio
Fix validation for duration vars on APM configuration settings (#124336…
3fb863a 
…) (#124514)

* Fix validation for duration vars on apm configuration settings

* Add unit test and just do the fix if the base package is apm

(cherry picked from commit c51daa7)

Co-authored-by: Miriam <[email protected]>
@joshdover
Copy link
Contributor

I could use some background on why this needed to change in the Fleet code rather than being handled in the APM policy UI or the migration from APM server -> integration policy code path.
Where did these text fields come from before? Was this a breaking change in the package input vars for APM?

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

2 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

sorenlouv
sorenlouv reviewed Feb 9, 2022
View reviewed changes
...ications/fleet/sections/agent_policy/edit_package_policy_page/utils/fix_apm_duration_vars.ts
Comment on lines +19 to +25
if (
entry[0] === IDLE_TIMEOUT ||
entry[0] === READ_TIMEOUT ||
entry[0] === SHUTDOWN_TIMEOUT ||
entry[0] === TAIL_SAMPLING_INTERVAL ||
entry[0] === WRITE_TIMEOUT
) {
Copy link
Member

Choose a reason for hiding this comment

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

@MiriamAparicio if you make DURATION_APM_SETTINGS_VARS an array you can simplify this to:

if (DURATION_APM_SETTINGS_VARS.includes(entry[0])) {

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

2 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@ogupte
Copy link
Contributor

ogupte commented Feb 14, 2022

@joshdover: I could use some background on why this needed to change in the Fleet code rather than being handled in the APM policy UI or the migration from APM server -> integration policy code path. Where did these text fields come from before? Was this a breaking change in the package input vars for APM?

The onChange handler passed to the UI extensions is called when the text inputs change, as a user types, this also means that the isValid flag is passed to the handler at the same time. This pattern suggests that the updatedPolicy acts as a staging area for settings updates that gets updated very frequently.

It's certainly possible to contain everything in the APM extension code without leaking into the fleet UI code, but we would essentially have to duplicate the 'staging area' pattern of that is already implemented in the fleet UI (updatedPolicy) in order to achieve the field-level granularity of validation rather than input-level like it is today.

@ogupte ogupte mentioned this pull request Feb 14, 2022
Open
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv left review comments

@juliaElastic juliaElastic juliaElastic approved these changes

@ogupte ogupte ogupte approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team v8.0.0 v8.0.1 v8.1.0 v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Warning in APM Integration Editor caused by invalid default duration
8 participants
@MiriamAparicio @elasticmachine @juliaElastic @kibana-ci @kibanamachine @joshdover @ogupte @sorenlouv