-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat(alexaSkill): add alexaSkill event type #20
Conversation
Adds support for Alexa Intent, and Alexa Start/End Session event types. Adds idx dev dependency for terse yet performant event-type check fix iopipe#17
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.
Need some context for this one. How would I use this skill? (I have an Echo)
Don't think this was on the roadmap, but like the idea all the same.
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 Corey this was fast! Catching up on reviews post-conference.
Just making sure I know what data's being collected here. If it's all the event records being logged I'll look into if there's any that might be sensitive data for users.
src/plugins/alexaSkill.js
Outdated
|
||
function plugin(event, log) { | ||
const obj = flatten(event); | ||
Object.keys(obj).forEach(key => { |
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 we're logging all of the keys?
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 looks like keys we don't want capture (at least):
@iopipe/event-info.alexaSkill.context.System.apiAccessToken
@iopipe/event-info.alexaSkill.context.System.user.accessToken
@iopipe/event-info.alexaSkill.context.System.user.permissions.consentToken
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 blacklist those, thanks @ewindisch makes it easy to update!
@ewindisch @kolanos back in review, added blacklisted keys requested by @ewindisch |
@coreylight @ewindisch Made equivalent updates here: iopipe/iopipe-python#218 |
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 we update the readme? Everything else looks great.
Signed-off-by: Michael Lavers <kolanos@gmail.com>
idx
dev dependency for terse yet performant event-type checkflat
dep for capturing unanticipated object key/valuesfix #17