-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Trace overview queries #23605
Trace overview queries #23605
Conversation
5f68737
to
140cc5b
Compare
Pinging @elastic/apm-ui |
💔 Build Failed |
💔 Build Failed |
retest |
💔 Build Failed |
💔 Build Failed |
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.
This looks super good! Only a few nits 👍
} | ||
|
||
export function ImpactBar({ value, ...rest }: Props) { | ||
export function ImpactBar({ value, range = [], ...rest }: Props) { | ||
const maxImpact = Math.max(0, ...range.map(({ impact }) => impact)); |
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 think max={max}
makes a little more sense to me than range={items}
. When looking at the component from the outside, I was not sure what range
did.
There is also the perf consideration that the max will be calculated for every single item.
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.
Yeah it might need a better name. The same calculation has to happen in one place or the other, though, whether it gets defined outside this component each time it's used or just defined the one time here, which is why I opted to move it in. I'll think about this over the weekend 🤔
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.
The same calculation has to happen in one place or the other, though, whether it gets defined outside this component each time it's used or just defined the one time here, which is why I opted to move it in
I don't think that's right. Right now maxImpact
is defined inside ImpactBar
, which will be rendered for every item in the list. So if there are 100 items, it will be calculated 100 times. If maxImpact
is defined outside ImpactBar
it will only be calculated once, regardless how many items there are.
Additionally I think we need a minImpact
also. We used to calculate it like this (slightly convoluted)
https://github.com/sqren/kibana/blob/6044cfb463473848af3225d97f95754be5538b5a/x-pack/plugins/apm/public/store/reactReduxRequest/transactionList.js#L17-L21
The minImpact
exaggerates small differences for high values.
Eg. If all values are between 90 and 100
Sample value: 91
Previous impact: 10%
New impact: 91%
Sample value: 95
Previous impact: 50%
New impact: 95%
Perhaps it would be simpler to calculate this upfront in node?
https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/server/lib/transactions/get_top_transactions.js#L87
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.
Something like this would work:
function getRelativeImpact(value, min, max) {
return (value - min) / (max - min)
}
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 see what you're saying now about having to calculate the same value for every item, good catch. The need for a minimum makes sense too. I'll work on something.
noItemsMessage: any; | ||
} | ||
|
||
export function TraceList({ items = [], noItemsMessage }: Props) { | ||
return ( | ||
<ManagedTable | ||
columns={TRACE_COLUMNS} | ||
columns={getTraceColumns({ items })} |
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.
Probably not a big issue, but before TRACE_COLUMNS
was defined outside the render function, and thus ManagedTable
would not re-render whenever TraceList
re-rendered. Now getTraceColumns
returns a new object every time TraceList
re-renders , which will cause ManagedTable
to re-render also.
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.
Ah good point. I think the only way to do this here would be to go back to a class component and build the columns in cDM or the constructor, which seems like a bad reason to convert to me. I wonder if there's a better way to manage this.
bool: { | ||
must: { | ||
exists: { | ||
field: 'trace.id' // todo: move to constant? or TS? |
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 thought we could get rid of the constants.js but for these cases I think the constants still make sense.
I was actually going to make a test that checks the constants against our typescript interfaces, so we are sure they don't get out of sync.
so tldr: unless you know a way to do this with typescript, use a constant.
}); | ||
|
||
// Sort results by impact - needs to be desc, hence the reverse() | ||
return sortBy(results, o => o.impact).reverse(); |
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.
We do actually have _.orderBy that supports sort order:
Line 148 in 4126d51
"lodash.orderby": "4.6.0", |
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.
Ah yeah this was just all moved over from the get_top_transactions file
I was considering making "get top traces" (maybe renamed "get trace list") to just use get transactions under the hood, with maybe an option to adjust the query slightly, since so much of those files are now duplicate. Thoughts?
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.
Ah, I love pointing out my own mistakes in other people's PRs :D
@jasonrhodes regarding #23605 (comment) I think you need the "closes" keyword in the PR description for it to work (normally "closes" gets squiggly lines) |
💔 Build Failed |
💔 Build Failed |
Ping @sqren -- I think this addresses most of the feedback. Went with a Also made a |
@@ -17,6 +17,7 @@ export const TRANSACTION_RESULT = 'transaction.result'; | |||
export const TRANSACTION_NAME = 'transaction.name'; | |||
export const TRANSACTION_ID = 'transaction.id'; | |||
export const TRANSACTION_SAMPLED = 'transaction.sampled'; | |||
export const PARENT_ID = 'parent.id'; // deprecated |
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.
parent.id
is not deprecated. It was added in v2:
https://github.com/elastic/apm-server/blob/v2/docs/fields.asciidoc#service-fields
span.parent
will be deprecated in v2 - maybe you're thinking of that?
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.
Yep. Was also looking at it being in the "must not exist" section of the query but I remember why now.
x-pack/plugins/apm/public/components/shared/ImpactBar/index.tsx
Outdated
Show resolved
Hide resolved
@@ -23,7 +24,7 @@ export function TraceOverview(props: Props) { | |||
<EuiSpacer /> | |||
<TraceListRequest | |||
urlParams={urlParams} | |||
render={({ data }: { data: object[] }) => ( | |||
render={({ data }: { data: ImpactItem[] }) => ( |
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.
Doesn't TraceListRequest
return a list of trace items, and not impact items?
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.
Btw. it would be cool if you could add an interface to the node endpoint, and re-use that here! :)
tracesPerMinute: tpm, | ||
impact | ||
}) | ||
}); |
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 think this level of abstraction makes the code harder to read, and think what you had before was better.
Personally I'm okay with less DRY code if it makes it more readable - especially when the duplication is only in two places.
Perhaps you can extract just the aggregation?
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.
Btw. I can totally related to this. When I first started in Elastic I wrote a DSL to get rid of the duplication in ES queries. I definitely went way further than you did :)
https://github.com/elastic/x-pack-kibana/pull/2653
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.
Yeah, I find this easier to understand and follow because it makes it clear what's different and what's not and helps confirm that traces are transactions, but I know abstraction has a trade-off cost. I don't mind repeating things, but I'm worried we're introducing a fair amount of code to retrieve "traces" that is actually just getting transactions with slightly different criteria to query for. What I think I'd really prefer is removing the transformResult
altogether and aligning the results from both queries to have the same keys, which would make this even more streamlined.
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 see. I think if you can get rid of transformResult
it would make it a lot more readable 👍
💔 Build Failed |
💔 Build Failed |
@@ -17,6 +17,10 @@ export const TRANSACTION_RESULT = 'transaction.result'; | |||
export const TRANSACTION_NAME = 'transaction.name'; | |||
export const TRANSACTION_ID = 'transaction.id'; | |||
export const TRANSACTION_SAMPLED = 'transaction.sampled'; | |||
export const TRANSACTION_SERVICE_NAME = 'context.service.name'; |
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.
This already exists as SERVICE_NAME
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.
Is this resolved?
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.
Hm must have been reverted during rebase, re-removed it.
}: { | ||
response: ITransactionGroupQueryResult; | ||
durationMinutes: number; | ||
}) { |
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 think prepareTransactionGroups
should just take buckets
and not the full response. Then you can get rid of ITransactionGroupQueryResult
.
btw. I added elasticsearch types, and it gives some pretty nice safety with few explicit types: https://github.com/sqren/kibana/blob/65a93973f823741e48894aa2875b819f03fb0a79/x-pack/plugins/apm/server/lib/traces/get_trace.ts#L62-L65
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 can do that, but I don't understand the difference?
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.
Oh you're saying to replace the custom query result type with the ES type.
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 don't think prepareTransactionGroups
should care about the structure of the ES response. All it cares about is the buckets.
I think we should be careful not overtyping everything. When we make an interface typescript will blindly trust us, and I think it mostly makes sense to have interfaces when they are used multiple places.
In this case we can get rid of ITransactionGroupQueryResult
if it only takes the buckets.
name: string; | ||
}; | ||
}; | ||
} |
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.
We should extract transaction and context from here and then do something like:
interface ITransactionGroupSample {
transaction: TransactionTransaction;
context: TransactionContext;
}
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.
Btw. I'm very open to better names than TransactionTransaction
:D
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.
... even TransactionDocTransaction
is better but that would require us to rename the Transaction
interface to TransactionDoc
If you rebase with |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
exists: { | ||
field: TRACE_ID | ||
} | ||
}, |
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.
Errors can also have trace_id
, so we should probably make sure it's a transaction:
{ term: { [PROCESSOR_EVENT]: 'transaction' } }
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.
Oh ok. Small confusion here: we don't currently do this same check in get_top_transactions
(we seem to check for a single transaction type instead, perhaps?) and the get_error_groups query searches on a different index config.get('apm_oss.errorIndices')
. What am I missing here and what's PROCESSOR_EVENT
?
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.
we don't currently do this same check in get_top_transactions (we seem to check for a single transaction type instead, perhaps?)
Yes, error don't have transaction.type
field. But to make it obvious we should probably add processor.event
there as well.
get_error_groups query searches on a different index config.get('apm_oss.errorIndices')
The default index is apm-*
:
https://github.com/elastic/kibana/blob/master/src/core_plugins/apm_oss/index.js#L33
Which is why we add:
{ term: { [PROCESSOR_EVENT]: 'error' } }
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.
What am I missing here and what's PROCESSOR_EVENT
Processor event is basically the document type. Valid types are transaction
, span
and error
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.
Thanks for clearing that up. I'll add processor event to both.
yarn.lock
Outdated
@@ -876,6 +876,10 @@ angular-mocks@1.4.7: | |||
version "1.4.7" | |||
resolved "https://registry.yarnpkg.com/angular-mocks/-/angular-mocks-1.4.7.tgz#d7343ee0a033f9216770bda573950f6814d95227" | |||
|
|||
angular-paging@2.2.1: |
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.
What is this?
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.
Came in when I rebased apm/dt off of master, then merged apm/dt into this branch. Not sure what it is.
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.
Let me know when this is rebased and I'll give it a last look.
3e1e910
to
dd99be5
Compare
package.json
Outdated
@@ -69,6 +69,7 @@ | |||
"@kbn/pm": "link:packages/kbn-pm", | |||
"@kbn/test-subj-selector": "link:packages/kbn-test-subj-selector", | |||
"@kbn/ui-framework": "link:packages/kbn-ui-framework", | |||
"@types/elasticsearch": "^5.0.26", |
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.
We already have this in devDeps: https://github.com/elastic/kibana/blob/apm/dt/package.json#L225
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.
Forgot the --dev
flag :headdesk:
Refactoring includes: * ImpactBar component to align EuiProgress usage for impact bars * Sharing some logic between transaction and trace queries * Typescript support
c2e1b6d
to
9adcdf3
Compare
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.
Looks good!
transactionType, | ||
serviceName | ||
}: { | ||
setup: StringMap<any>; |
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.
You could use the new Setup
type. But don't worry about it now if you get a green build. We can always fix that later.
const duration = moment.duration(end - start); | ||
const minutes = duration.asMinutes(); | ||
|
||
const results = buckets.map((bucket: ITransactionGroupBucket) => { |
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.
Since buckets
are typed, bucket
should be inferred. But don't worry about that now.
💚 Build Succeeded |
💚 Build Succeeded |
const params = { | ||
index: config.get('apm_oss.transactionIndices'), | ||
body: { | ||
size: 'banana', |
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.
@jasonrhodes This is causing:
{"statusCode":400,"error":"Bad Request","message":"[number_format_exception] For input string: \"banana\""}
Refactoring includes: * ImpactBar component to align EuiProgress usage for impact bars * Sharing some logic between transaction and trace queries * Typescript support
* Adds traces overview with mock data (#22628) * Updates service overview snapshots * Adds tests for ManagedTable and ImpactBar * Refactored transaction overview to use new managed table component * Removed jsconfig file in apm * [APM] Distributed tracing - Trace details (waterfall) (#22763) * [APM] Add typescript to waterfall (#23635) * [APM] Migrate get_trace and constants to Typescript (#23634) * [APM] Add types for setup_request (#23762) * [APM] Adds trace overview queries and some refactoring (#23605) * ImpactBar component to align EuiProgress usage for impact bars * Sharing some logic between transaction and trace queries * Typescript support * Quick fix ‘banana’ * [APM] Ensure backwards compatibility for v1 and v2 (#23636) * Make interfaces versioned * Rename eventType to docType * Fixes trace links on traces overview (#24089) * [APM] use react-redux-request (#24117) * Updated yarn lockfile for new yarn version * Updated dependency issues for react-router-dom types * [APM] Display transaction info on span flyout (#24189) * [APM] Display transaction info on span flyout * Brings in real location and url param data for transaction flyout * Converts flyout to TS * Adds query param state for flyouts with ts support * Updates styles and uses EuiTabs for transaction flyout * [APM] Transaction flyout * [APM] Minor docs cleanup (#24325) * [APM] Minor docs cleanup * [APM] Fix issues with v1 spans (#24332) * [APM] Add agent marks (#24361) * [APM] Typescript migration for the transaction endpoints (#24397) * [APM] DT transaction sample header (#24294) Transaction sample header completed * Fixes link target for traces overview to include trans/trace ids as query params * Converts Transaction index file to TS * Adds trace link to sample section * Refactors the trace link and applies it to two usages * Implements transaction sample action context menu * Calculates and implements duration percentage * Re-typed how transaction groups work * Fixes transaction flyout links and context menu * Removes unnecessary ms multiplication * Removes unused commented code * Finalizes infra links * Fixes some type shenanigans
* Adds traces overview with mock data (#22628) * Adds traces overview with mock data * Updates service overview snapshots * Updates based on review feedback * Adds tests for ManagedTable and ImpactBar * Refactored transaction overview to use new managed table component * Fixes tab size * Cleans up some tests and types * Cleans up types and tests * kbn bootstrap changes to x-pack yarn.lock * Removed jsconfig file in apm * Updates snapshot tests * Reversed bogus yarn lock change in x-pack * review feedback wip * Resolves typescript issues * [APM] Distributed tracing - Trace details (waterfall) (#22763) * [APM] Add typescript to waterfall (#23635) * [APM] Migrate get_trace and constants to Typescript (#23634) * Bump lock files * [APM] Add types for setup_request (#23762) * [APM] Adds trace overview queries and some refactoring (#23605) Refactoring includes: * ImpactBar component to align EuiProgress usage for impact bars * Sharing some logic between transaction and trace queries * Typescript support * Quick fix ‘banana’ * [APM] Ensure backwards compatibility for v1 and v2 (#23636) Fix tests Make interfaces versioned Fix ts issues Rename eventType to docType WIP Working prototype Fix tests Limit get_transaction by trace.id * Fixes trace links on traces overview (#24089) * [APM] use react-redux-request (#24117) * [APM] use react-redux-request * Stricter type checking * Bump yarn lock files * Fix package.json * Updated yarn lockfile for new yarn version * Updated dependency issues for react-router-dom types * [APM] Display transaction info on span flyout (#24189) * [APM] Display transaction info on span flyout * Add links * Fix tests * Eui fixes * Remove color from span type * Fix failing tests * Adds versions to transaction and span itmes when they are returned from the API (#24186) * Timeline transaction flyout (#24027) * Duplicates transaction properties table for use in flyout, with dummy values * Brings in real location and url param data for transaction flyout * Converts flyout to TS * Adds query param state for flyouts with ts support * Updates styles and uses EuiTabs for transaction flyout * Updates type references after rebase * Updated index var name per review feedback * WIP transaction flyout * WIP transaction flyout * WIP * Fixes waterfall helpers after rebasing span flyout changes * Finalizes styling for transaction flyover from timeline * Fixes layout for span flyout header * Removed accidentally committed file (should be in other branch) * Small tweaks to the sticky property layout styling * Updates snapshot tests after sticky properties converting to EUI * Final review tweaks for TS * [APM] Minor docs cleanup (#24325) * [APM] Minor docs cleanup * Fix tests * Make `text` required * Simplify documentation module * Break docs into separate files * Remove `translateAgentName` method * Formatting * [APM] Fix issues with v1 spans (#24332) * [APM] Add agent marks (#24361) * Typescript migration for the transaction endpoints (#24397) * DT transaction sample header (#24294) Transaction sample header completed * Fixes link target for traces overview to include trans/trace ids as query params * Converts Transaction index file to TS * Adds trace link to sample section * Refactors the trace link and applies it to two usages * Implements transaction sample action context menu * Calculates and implements duration percentage * Re-typed how transaction groups work * Fixes transaction flyout links and context menu * Removes unnecessary ms multiplication * Removes unused commented code * Finalizes infra links * Fixes some type shenanigans
* Adds traces overview with mock data (#22628) * Updates service overview snapshots * Adds tests for ManagedTable and ImpactBar * Refactored transaction overview to use new managed table component * Removed jsconfig file in apm * [APM] Distributed tracing - Trace details (waterfall) (#22763) * [APM] Add typescript to waterfall (#23635) * [APM] Migrate get_trace and constants to Typescript (#23634) * [APM] Add types for setup_request (#23762) * [APM] Adds trace overview queries and some refactoring (#23605) * ImpactBar component to align EuiProgress usage for impact bars * Sharing some logic between transaction and trace queries * Typescript support * Quick fix ‘banana’ * [APM] Ensure backwards compatibility for v1 and v2 (#23636) * Make interfaces versioned * Rename eventType to docType * Fixes trace links on traces overview (#24089) * [APM] use react-redux-request (#24117) * Updated yarn lockfile for new yarn version * Updated dependency issues for react-router-dom types * [APM] Display transaction info on span flyout (#24189) * [APM] Display transaction info on span flyout * Brings in real location and url param data for transaction flyout * Converts flyout to TS * Adds query param state for flyouts with ts support * Updates styles and uses EuiTabs for transaction flyout * [APM] Transaction flyout * [APM] Minor docs cleanup (#24325) * [APM] Minor docs cleanup * [APM] Fix issues with v1 spans (#24332) * [APM] Add agent marks (#24361) * [APM] Typescript migration for the transaction endpoints (#24397) * [APM] DT transaction sample header (#24294) Transaction sample header completed * Fixes link target for traces overview to include trans/trace ids as query params * Converts Transaction index file to TS * Adds trace link to sample section * Refactors the trace link and applies it to two usages * Implements transaction sample action context menu * Calculates and implements duration percentage * Re-typed how transaction groups work * Fixes transaction flyout links and context menu * Removes unnecessary ms multiplication * Removes unused commented code * Finalizes infra links * Fixes some type shenanigans
Closes #23438