-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[UX Dashboard] Migrate service list query out of APM #132548
Changes from all commits
60cba17
0d280fa
5c2f9bd
335bbd5
6b1689d
a0477c5
98724ab
f219fef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
export enum ProcessorEvent { | ||
transaction = 'transaction', | ||
error = 'error', | ||
metric = 'metric', | ||
span = 'span', | ||
profile = 'profile', | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fine for now. But we should find a way to share these to avoid duplication. Eg. this could reside in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++ I am happy to move them there now and reference from APM if you want. I can make that as a follow-up so it's easier to review without these tangential changes. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { DeepPartial } from 'utility-types'; | ||
import { cloneDeep, isPlainObject, mergeWith } from 'lodash'; | ||
|
||
type PlainObject = Record<string | number | symbol, any>; | ||
|
||
type SourceProjection = DeepPartial<any>; | ||
|
||
type DeepMerge<T, U> = U extends PlainObject | ||
? T extends PlainObject | ||
? Omit<T, keyof U> & { | ||
[key in keyof U]: T extends { [k in key]: any } | ||
? DeepMerge<T[key], U[key]> | ||
: U[key]; | ||
} | ||
: U | ||
: U; | ||
|
||
export function mergeProjection<T extends any, U extends SourceProjection>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ended up moving this over because there's logic about projection in the query generator for this component. LMK if this can be somehow skipped and we can delete this file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. APM doesn't use projections anymore. If you also want to get rid of projections for the UX related code you are welcome to do it as part of this PR or a follow-up. If you do that we can delete the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sqren I think there are one or two other components in APM relying on that code. When I take them out I'll make sure nothing else references the projections and delete then. @shahzad31 I'd appreciate your thoughts on whether we want this projection code to move into UX or if we can remove it here and now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think if it makes sense we can move these. If you think the util apm is using in place of projections, it's also worth exploring those. |
||
target: T, | ||
source: U | ||
): DeepMerge<T, U> { | ||
return mergeWith({}, cloneDeep(target), source, (a, b) => { | ||
if (isPlainObject(a) && isPlainObject(b)) { | ||
return undefined; | ||
} | ||
return b; | ||
}) as DeepMerge<T, U>; | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { getEsFilter } from './get_es_filter'; | ||
|
||
describe('getEsFilters', function () { | ||
it('should return environment in include filters', function () { | ||
const result = getEsFilter({ | ||
browser: ['Chrome'], | ||
environment: 'production', | ||
}); | ||
|
||
expect(result).toEqual([ | ||
{ terms: { 'user_agent.name': ['Chrome'] } }, | ||
{ term: { 'service.environment': 'production' } }, | ||
]); | ||
}); | ||
|
||
it('should not return environment in exclude filters', function () { | ||
const result = getEsFilter( | ||
{ browserExcluded: ['Chrome'], environment: 'production' }, | ||
true | ||
); | ||
|
||
expect(result).toEqual([{ terms: { 'user_agent.name': ['Chrome'] } }]); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
/* | ||
justinkambic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { ESFilter } from '@kbn/core/types/elasticsearch'; | ||
import { ENVIRONMENT_ALL } from '../../../common/environment_filter_values'; | ||
import { | ||
uxLocalUIFilterNames, | ||
uxLocalUIFilters, | ||
} from '../../../common/ux_ui_filter'; | ||
import { UxUIFilters } from '../../../typings/ui_filters'; | ||
import { environmentQuery } from '../../components/app/rum_dashboard/local_uifilters/queries'; | ||
|
||
export function getEsFilter(uiFilters: UxUIFilters, exclude?: boolean) { | ||
const localFilterValues = uiFilters; | ||
const mappedFilters = uxLocalUIFilterNames | ||
.filter((name) => { | ||
const validFilter = name in localFilterValues; | ||
if (typeof name !== 'string') return false; | ||
if (exclude) { | ||
return name.includes('Excluded') && validFilter; | ||
} | ||
return !name.includes('Excluded') && validFilter; | ||
}) | ||
.map((filterName) => { | ||
const field = uxLocalUIFilters[filterName]; | ||
const value = localFilterValues[filterName]; | ||
|
||
return { | ||
terms: { | ||
[field.fieldName]: value, | ||
}, | ||
}; | ||
}) as ESFilter[]; | ||
|
||
return [ | ||
...mappedFilters, | ||
...(exclude | ||
? [] | ||
: environmentQuery(uiFilters.environment || ENVIRONMENT_ALL.value)), | ||
]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the remaining routes will have to be moved out in a future PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sqren yes we are going to be doing this one component at a time, or grouped if there are some that it makes sense to move at once. Some are going to be ported to using data plugin queries, others will become embeddables, with the goal of avoiding adding a server component to the UX plugin. We're planning to have them all removed soon!