-
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
Dashboard favorites telemetry #190706
Dashboard favorites telemetry #190706
Conversation
…kibana into d/2024-08-14-favorites-telemetry
…vorites-telemetry
…vorites-telemetry
hidden: true, | ||
namespaceType: 'single', | ||
mappings: { | ||
dynamic: false, | ||
properties: {}, | ||
properties: { |
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 didn't need these mappings for the feature, but, unfortunately, I didn't know that we would need them for the snapshot telemetry. I am not sure if there is another better way
the object schema stays the same, but we add existing fields to the mapping
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 been my experience as well that it's sometimes necessary to add mappings just for the sake of being able to collect snapshot telemetry.
number_of_favorites: { | ||
type: 'long', | ||
script: { | ||
source: "emit(doc['favorites.favoriteIds'].length)", |
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 if this is the best way, but I like how clean it is to get sum, avg, max.
I've also tried value_count aggregation which works well for total, but haven't figure how it can be used for avg and max
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.
If you need other alternatives to consider, there is also the stats aggregation.
Running the aggregation on a runtime field seems like a perfect idea for 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.
Thanks, I updated to stats
…kibana into d/2024-08-14-favorites-telemetry
/ci |
Pinging @elastic/appex-sharedux (Team:SharedUX) |
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.
Core changes LGTM
@@ -11,6 +11,7 @@ | |||
"../../../typings/**/*", | |||
"schema/oss_plugins.json", | |||
"schema/oss_root.json", | |||
"schema/kbn_packages.json" |
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!
@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.
type: 'long', | ||
_meta: { | ||
description: | ||
'Total unique users+spaces that have favorited an object of this type in this deployment', |
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 follow what users+spaces
means. Does it mean users per space?
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'll adjust!
|
||
export function registerFavoritesUsageCollection({ | ||
core, | ||
logger, |
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.
Should logger
be used for anything?
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.
cleaned it up
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.
LGTM
Added a few conversational comments
…vorites-telemetry # Conflicts: # src/plugins/content_management/tsconfig.json
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Public APIs missing comments
Async chunks
Saved Objects .kibana field count
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Add telemetry to favorites feature #189285
Unfortunately, for snapshot telemetry, I had to add fields to kibana mapping. We didn't need them for a feature, but I didn't realize that will have to add them to a mapping. Not sure if there is a better way