-
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
Instrument Index Management with user action telemetry #32595
Instrument Index Management with user action telemetry #32595
Conversation
- Use constants instead of hard-coded strings to identify tabs. - Internationalize tab labels.
Pinging @elastic/es-ui |
💚 Build Succeeded |
- Track refresh action. - Rename 'bulk' action type to 'many' for clarity.
@bmcconaghy I've updated the PR per our discussion by removing the |
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. I think the code is clear now in what is happening when. Thanks for working through this.
💔 Build Failed |
💔 Build Failed |
Retest |
💚 Build Succeeded |
* Fix API endpoint typo indices/clear_caches -> indices/clear_cache. * Track Index Management index actions. * Track user actions for opening detail tab and viewing various tabs. - Use constants instead of hard-coded strings to identify tabs. - Internationalize tab labels. * Track update settings action. * Track refresh action. * Track app load.
* Fix API endpoint typo indices/clear_caches -> indices/clear_cache. * Track Index Management index actions. * Track user actions for opening detail tab and viewing various tabs. - Use constants instead of hard-coded strings to identify tabs. - Internationalize tab labels. * Track update settings action. * Track refresh action. * Track app load.
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.
@cjcenizal Although this has been merged I think we should address the 2 comments I made. One is about React component lifecycle deprecated and the other is to maintain consistency across our apps. We can discuss over zoom if you want. Thanks!
</div> | ||
); | ||
export class App extends Component { | ||
componentWillMount() { |
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 lifecycle is deprecated (https://reactjs.org/blog/2018/03/29/react-v-16-3.html). We should use componentDidMount()
@@ -32,6 +66,9 @@ export async function closeIndices(indices) { | |||
indices | |||
}; | |||
const response = await httpClient.post(`${apiPrefix}/indices/close`, body); |
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.
For consistency, I think it would have been better:
const actionType = indices.length > 1 ? UA_INDEX_CLOSE_MANY : UA_INDEX_CLOSE;
const request = httpClient.post(`${apiPrefix}/indices/close`, body);
const response = await trackUserRequest(request, actionType);
...
You know, I'm all about consistency 😊 Aside, the trackUserRequest()
(that will come as a public method from the userAction plugin in the new platform) being an abstraction, we could easily extend it's behaviour in in a single place whenever needed.
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 discussed this, found some ways to make trackUserRequest
easier to understand, and agreed it's OK to have different implementations from app-to-app for now, with an eye towards unifying them in the future once the New Platform is in a state that supports this effort.
Actions tracked:
Telemetry shape as reported by
http://localhost:5601/api/stats?extended=true
: