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

Remove environment from uiFilters #89647

Merged
merged 34 commits into from
Feb 17, 2021
Merged

Remove environment from uiFilters #89647

merged 34 commits into from
Feb 17, 2021

Conversation

smith
Copy link
Contributor

@smith smith commented Jan 28, 2021 •
edited
Loading

  • Explicitly use environment from query string instead of uiFilters
  • Remove environment from setup.esFilter
  • Replace getEnvironmentUiFilterES with environmentQuery
  • Replace rangeFilter with rangeQuery

@smith
Copy link
Contributor Author

smith commented Feb 2, 2021

@elasticmachine merge upstream

@smith
Copy link
Contributor Author

smith commented Feb 2, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@spalger
Copy link
Contributor

spalger commented Feb 4, 2021

jenkins test this

@smith smith mentioned this pull request Feb 5, 2021
Closed
8 tasks
smith
smith commented Feb 5, 2021
View reviewed changes
x-pack/plugins/apm/server/lib/transaction_groups/__snapshots__/queries.test.ts.snap
@@ -240,12 +240,8 @@ Array [
"bool": Object {
"filter": Array [
Object {
"range": Object {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes here are only a reflection in the changes of ordering the filters for readability.

smith
smith commented Feb 5, 2021
View reviewed changes
x-pack/plugins/apm/server/routes/create_apm_api.ts
@@ -165,8 +165,8 @@ const createApmApi = () => {
.add(transactionChartsErrorRateRoute)
.add(transactionGroupsRoute)
.add(transactionGroupsOverviewRoute)
.add(transactionLatencyChatsRoute)
.add(transactionThroughputChatsRoute)
.add(transactionLatencyChartsRoute)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed misspelling here.

shahzad31
shahzad31 reviewed Feb 11, 2021
View reviewed changes
x-pack/plugins/apm/server/lib/rum_client/has_rum_data.ts
filter: {
range: rangeFilter(start, end),
},
filter: rangeQuery(start, end)[0],
Copy link
Contributor

Choose a reason for hiding this comment

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

rangeQuery returns an array or object?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it returns array, i guess it can be assigned directly here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These query functions return arrays because they are normally spread. This is because filter: [{}] is an error in Elasticsearch queries but filter: [...[]] is not.

shahzad31
shahzad31 approved these changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

UX App changes LGTM!!

sorenlouv
sorenlouv reviewed Feb 17, 2021
View reviewed changes
x-pack/plugins/apm/common/utils/queries.test.ts
});

it('creates a query for missing service environments', () => {
expect(environmentQuery(ENVIRONMENT_NOT_DEFINED.value)[0]).toHaveProperty(
Copy link
Member

Choose a reason for hiding this comment

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

Definitely a better name 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
apm 5.2MB 5.2MB +1.0KB

History

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

@smith smith merged commit f650c38 into elastic:master Feb 17, 2021
@smith smith deleted the nls/env-filters branch February 17, 2021 20:28
@kibanamachine kibanamachine mentioned this pull request Feb 17, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 17, 2021
@smith @kibanamachine
Remove environment from uiFilters (elastic#89647)
40d01a6
@kibanamachine
Copy link
Contributor

💚 Backport successful

✅ 7.x / #91745

Successful backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Feb 17, 2021
@kibanamachine @smith
Remove environment from uiFilters (#89647) (#91745)
828aa1b 
Co-authored-by: Nathan L Smith <nathan.smith@elastic.co>
This was referenced Feb 19, 2021
Merged
Closed
Merged
smith added a commit to smith/kibana that referenced this pull request Mar 3, 2021
@smith
Fix service map for All environment single service
fef1f52 
Before we removed environment from the UI filters (elastic#89647), the environment query parameter would be undefined if "All" was selected. Now we send ENVIRONMENT_ALL in as the query parameter.

Changes in https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/server/lib/service_map/get_service_map_from_trace_ids.ts made it so no connections would be returned if ENVIRONMENT_ALL was selected, rather than all connections. Since no connections were being returned, no elements except the selected service would be returned in the API response.

This changes it so if ENVIRONMENT_ALL is selected, the connection will always be returned, just like what used to be the case when environment was undefined.

Add an API test for this case.

Fixes elastic#93385.
@smith smith mentioned this pull request Mar 3, 2021
Merged
smith added a commit that referenced this pull request Mar 3, 2021
@smith
Fix service map for All environment single service (#93517)
4e21fae 
Before we removed environment from the UI filters (#89647), the environment query parameter would be undefined if "All" was selected. Now we send ENVIRONMENT_ALL in as the query parameter.

Changes in https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/server/lib/service_map/get_service_map_from_trace_ids.ts made it so no connections would be returned if ENVIRONMENT_ALL was selected, rather than all connections. Since no connections were being returned, no elements except the selected service would be returned in the API response.

This changes it so if ENVIRONMENT_ALL is selected, the connection will always be returned, just like what used to be the case when environment was undefined.

Add an API test for this case.

Fixes #93385.
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 3, 2021
@smith @kibanamachine
Fix service map for All environment single service (elastic#93517)
c43c0a2 
Before we removed environment from the UI filters (elastic#89647), the environment query parameter would be undefined if "All" was selected. Now we send ENVIRONMENT_ALL in as the query parameter.

Changes in https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/server/lib/service_map/get_service_map_from_trace_ids.ts made it so no connections would be returned if ENVIRONMENT_ALL was selected, rather than all connections. Since no connections were being returned, no elements except the selected service would be returned in the API response.

This changes it so if ENVIRONMENT_ALL is selected, the connection will always be returned, just like what used to be the case when environment was undefined.

Add an API test for this case.

Fixes elastic#93385.
smith added a commit to smith/kibana that referenced this pull request Mar 3, 2021
@smith
Fix service map for All environment single service (elastic#93517)
775fa55 
Before we removed environment from the UI filters (elastic#89647), the environment query parameter would be undefined if "All" was selected. Now we send ENVIRONMENT_ALL in as the query parameter.

Changes in https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/server/lib/service_map/get_service_map_from_trace_ids.ts made it so no connections would be returned if ENVIRONMENT_ALL was selected, rather than all connections. Since no connections were being returned, no elements except the selected service would be returned in the API response.

This changes it so if ENVIRONMENT_ALL is selected, the connection will always be returned, just like what used to be the case when environment was undefined.

Add an API test for this case.

Fixes elastic#93385.
kibanamachine added a commit that referenced this pull request Mar 4, 2021
@kibanamachine @smith
Fix service map for All environment single service (#93517) (#93534)
da2a7fc 
Before we removed environment from the UI filters (#89647), the environment query parameter would be undefined if "All" was selected. Now we send ENVIRONMENT_ALL in as the query parameter.

Changes in https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/server/lib/service_map/get_service_map_from_trace_ids.ts made it so no connections would be returned if ENVIRONMENT_ALL was selected, rather than all connections. Since no connections were being returned, no elements except the selected service would be returned in the API response.

This changes it so if ENVIRONMENT_ALL is selected, the connection will always be returned, just like what used to be the case when environment was undefined.

Add an API test for this case.

Fixes #93385.

Co-authored-by: Nathan L Smith <nathan.smith@elastic.co>
smith added a commit that referenced this pull request Mar 4, 2021
@smith
Fix service map for All environment single service (#93517) (#93535)
73948a3 
Before we removed environment from the UI filters (#89647), the environment query parameter would be undefined if "All" was selected. Now we send ENVIRONMENT_ALL in as the query parameter.

Changes in https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/server/lib/service_map/get_service_map_from_trace_ids.ts made it so no connections would be returned if ENVIRONMENT_ALL was selected, rather than all connections. Since no connections were being returned, no elements except the selected service would be returned in the API response.

This changes it so if ENVIRONMENT_ALL is selected, the connection will always be returned, just like what used to be the case when environment was undefined.

Add an API test for this case.

Fixes #93385.
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

@shahzad31 shahzad31 shahzad31 approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@smith @kibanamachine @spalger @elasticmachine @sorenlouv @shahzad31