Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add UI telemetry tracking to AS in Kibana #5
Add UI telemetry tracking to AS in Kibana #5
Changes from all commits
5e58251
757fe71
020ab86
cb9708f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Just wondering out loud if this will execute, or if the page will navigate away before this can execute?
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.
These are set on all
target="_blank"
links only, so it'll always work because the page itself isn't navigating away to anything.I'm aware there's nothing stopping another dev from using the sendTelemetry helper on a non target blank link, but I don't think it's worth trying to enforce that right now because eventually (when all of AS is on Kibana) we won't have any links opening to a new AS tab in any case, and will probably lose this telemetry action.
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.
Makes send to me.
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't even 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.
Is there any valid use case where
action
,metric
, orhttp
would actually change and we'd want to update the metrics? Just wondering if this should be[]
instead.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.
So this is an eslint thing, personally I do think it should be
[]
but when I set it to that eslint'sreact-hooks/exhaustive-deps
rule flags an error: facebook/react#14920There's a ton of discourse on it on the internet and I'm not a huge fan of the rule, but since it's Kibana's codebase we either play by their rules or put it in a lot of one-liner eslint disables 🤷
There's actually another TODO comment in engine_overview.tsx where I did end up disabling the line - I think there's another link in that comment that has more context/info.
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.
Gotcha. I mean i think it will be fine as is, I'm not concerned about 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.
Haha yea same, but I also personally wish that lint rule was a little less strict 😁
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.
Note to self - if we weren't specifically returning a mocked response in get, we could also use Kibana's
savedObjectsRepositoryMock
test helper (viaimport { savedObjectsRepositoryMock } from 'src/core/server/mocks';
), but it was as easy or faster to just stub out what we needed here