-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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] Mobile most launches #168925
[APM] Mobile most launches #168925
Conversation
Pinging @elastic/apm-ui (Team:APM) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
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.
AO changes LGTM
@elasticmachine merge upstream |
@elasticmachine merge upstream |
} | ||
|
||
export interface EventRaw extends APMBaseDoc { | ||
processor: Processor; |
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.
Does the EventRaw have the processor field?
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.
Good point, it doesn't. I'll remove it
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 updated now!
const aggs = { | ||
launches: { | ||
filter: { | ||
terms: { ['labels.lifecycle_state']: ['created', 'active'] }, |
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.
nit: Can you please convert labels.lifecycle_state
as constant. Similar https://github.com/LikeTheSalad/kibana/blob/3a48bc9289b7dc9b8ecdebbf224a55686a5078f1/x-pack/plugins/apm/common/es_fields/apm.ts#L192C1-L192C1
Also, the values created and active can be enum.
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've converted labels.lifecycle_state
to constant 👍 However, regarding creating enums for created
and active
, I'm not sure what could be a proper file to place them? Also, I guess we'd have to create 2 separate enums, one for android events which would contain created
and other for iOS's events with active
?
x-pack/plugins/apm/server/lib/helpers/create_es_client/create_apm_event_client/index.ts
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
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.
Great job, @LikeTheSalad! Thank you for your efforts.
I couldn't test the Android due to the lack of data in edge. I did review the iOS launches, and they seem great, except for the time series. I left a comment with my feedback.
response.aggregations?.timeseries?.buckets.map((bucket) => ({ | ||
x: bucket.key, | ||
y: | ||
response.aggregations?.launches?.byLocation?.buckets[0]?.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.
response.aggregations?.launches?.byLocation?.buckets[0]?.doc_count ?? | |
bucket.launches?.byLocation?.buckets[0]?.doc_count ?? 0 |
Otherwise, you'll always return the total launches for the timeseries.
it seems we have introduced this bug here
kibana/x-pack/plugins/apm/server/routes/mobile/get_mobile_http_requests_by_location.ts
Line 115 in 1697bbb
response.aggregations?.requests?.requestsByLocation?.buckets[0] |
can you please fix there as well 🙏
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.
Could you also please add an API test to prevent this issue? It appears that our existing tests were insufficient in identifying this problem
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.
Hi @kpatticha - I've just made these changes, fixed the same issue for get_mobile_http_requests_by_location.ts
and also I had to fix it in here as well:
kibana/x-pack/plugins/apm/server/routes/mobile/get_mobile_crashes_by_location.ts
Line 105 in 1697bbb
response.aggregations?.crashes?.crashesByLocation?.buckets[0] |
I've also updated the API test to verify the changes. It should be good now, cheers!
x-pack/plugins/apm/server/routes/mobile/get_mobile_launches_by_location.ts
Show resolved
Hide resolved
…_location.ts Co-authored-by: Katerina <kate@kpatticha.com>
@elasticmachine merge upstream |
x-pack/test/apm_api_integration/tests/mobile/mobile_location_stats.spec.ts
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Async chunks
Public APIs missing exports
History
To update your PR or re-run it, just comment with: |
Summary
Enabling the "Most launches" Mobile dashboard panel which shows an aggregation of log events that contain the attribute
labels.lifecycle_state
set to eithercreated
(for Android) oractive
(for iOS).Checklist
Delete any items that are not applicable to this PR.
For maintainers