-
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
[APM] Enable metric-powered ui #104929
[APM] Enable metric-powered ui #104929
Conversation
Pinging @elastic/apm-ui (Team:apm) |
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.
@@ -54,7 +54,7 @@ export default function ApiTest({ getService }: FtrProviderContext) { | |||
|
|||
expectSnapshot(response.body.serviceCount).toMatchInline(`9`); | |||
|
|||
expectSnapshot(response.body.transactionPerMinute.value).toMatchInline(`64.8`); | |||
expectSnapshot(response.body.transactionPerMinute.value).toMatchInline(`302.2`); |
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 seems like too big a difference to be right.
I'm wondering how trustworthy the metric documents in the archive are. Perhaps some improvements were made to metric doc generation after we built the archive?
@dgieselaar any idea?
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 the bug we've found with _doc_count?
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.
Not sure. Has that been fixed and would rebuilding the archives fix 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.
It's an es bug on the search / aggregation side, so rebuilding the archive won't matter. Not sure what the status is of the bug fix.
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.
It's fixed and backported to 7.x 11 days ago so I assumed it made 7.14
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.
It's a missing filter query, it aggregates over all metric documents, not just the transaction metrics one. I've pushed a fix.
@elasticmachine merge upstream |
...getDocumentTypeFilterForAggregatedTransactions( | ||
searchAggregatedTransactions | ||
), |
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.
Don't we have a metricset.name
that we can automatically add, similar to how we filter by processor.event when using the apm event client?
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.
Eg changing getProcessorEventForAggregatedTransactions
to getApmEventForAggregatedTransactions
:
export function getApmEventForAggregatedTransactions(
searchAggregatedTransactions: boolean
): MetricsetName.transactionMetric | ProcessorEvent.transaction {
return searchAggregatedTransactions
? MetricsetName.transactionMetric
: ProcessorEvent.transaction;
}
and use it like:
const params = {
apm: {
events: [
getApmEventForAggregatedTransactions(
searchAggregatedTransactions
)
],
},
body: {},
}
apmEventClient.search(operationName, params)
where apm.events
is a union of ProcessorEvent
and MetricsetName
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.
Opened #105713 to track this.
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/security_solution/hosts·ts.apis SecuritySolution Endpoints hosts Make sure that we get Last Seen for a Host without docValueFieldsStandard Out
Stack Trace
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @dgieselaar |
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Dario Gieselaar <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Dario Gieselaar <[email protected]> Co-authored-by: Søren Louv-Jansen <[email protected]> Co-authored-by: Dario Gieselaar <[email protected]>
Closes #92024