Skip to content
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

Iterate on Sentry filtering #7292

Merged
merged 60 commits into from
Jan 14, 2021
Merged

Iterate on Sentry filtering #7292

merged 60 commits into from
Jan 14, 2021

Conversation

m-vdb
Copy link
Collaborator

@m-vdb m-vdb commented Nov 17, 2020

Proposed changes:

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@m-vdb m-vdb changed the title ignore errors from PyMongo in Sentry Iterate on Sentry filtering Nov 17, 2020
@m-vdb
Copy link
Collaborator Author

m-vdb commented Nov 17, 2020

There are some issues coming up today into Sentry (1958583765, 1952538184, 1962256159, 1951854941, 1956288097, 1946342362) that come from schema validation issues of config / domain files. While exploring solutions, I've hit a roadblock with pykwalify (the lib we use to validate yaml against a schema), especially on the slot types definition: with this lib, it's not possible to validate an object against an union of schema ("any of" use case). I don't think it's the main limitation that we have today, would be happy to hear some feedback on this point.

To me the impact is at different levels:

  • At the user level: the code fails with an exception, not telling the user what the problem is. They might even think that rasa is broken
  • At the rasa engineer level: hard to pin-point the initial issue when supporting a user on the forum, noise in Sentry.

I've found an alternative, cerberus which seem to support all of our use cases and seems a bit more advance at first sight.

rasa/telemetry.py Outdated Show resolved Hide resolved
rasa/telemetry.py Outdated Show resolved Hide resolved
rasa/shared/utils/common.py Outdated Show resolved Hide resolved
@m-vdb
Copy link
Collaborator Author

m-vdb commented Nov 18, 2020

@tmbo could I get your feedback on this one? Nothing urgent and it should be quick for you I think ^^ (tests + changelog fragments are obviously missing for the draft)

Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot for digging into these issues 🚀 left some thoughts

rasa/shared/utils/common.py Show resolved Hide resolved
rasa/shared/utils/common.py Outdated Show resolved Hide resolved
rasa/shared/utils/common.py Outdated Show resolved Hide resolved
rasa/telemetry.py Outdated Show resolved Hide resolved
rasa/core/brokers/broker.py Outdated Show resolved Hide resolved
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great iteration! Waiting for the approval due to my comment about Rasa X implications.

rasa/core/tracker_store.py Outdated Show resolved Hide resolved
rasa/shared/nlu/training_data/entities_parser.py Outdated Show resolved Hide resolved
rasa/shared/nlu/training_data/entities_parser.py Outdated Show resolved Hide resolved
rasa/shared/exceptions.py Outdated Show resolved Hide resolved
rasa/shared/utils/common.py Outdated Show resolved Hide resolved
rasa/telemetry.py Outdated Show resolved Hide resolved
tests/core/test_tracker_stores.py Outdated Show resolved Hide resolved
@m-vdb
Copy link
Collaborator Author

m-vdb commented Jan 12, 2021

@wochinge thanks for all your review comments 💯 I think I addressed all your comments and there is this remaining open question before this can be merged

rasa/core/brokers/broker.py Show resolved Hide resolved
rasa/core/brokers/broker.py Outdated Show resolved Hide resolved
rasa/core/tracker_store.py Outdated Show resolved Hide resolved
rasa/shared/exceptions.py Show resolved Hide resolved
rasa/shared/exceptions.py Outdated Show resolved Hide resolved
rasa/shared/utils/common.py Outdated Show resolved Hide resolved
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improving the reporting and doing some maintenance work at the same time 💯

a Python class

Raises:
ImportError, in case the Python class cannot be found.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically also a TypeError in case import_module fails

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I follow 🤔 doesn't it raise a ModuleNotFoundError which inherits ImportError?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import_module in 3.8.6 also raises a TypeError which inherits from Exception

def import_module(name, package=None):
    """Import a module.

    The 'package' argument is required when performing a relative import. It
    specifies the package to use as the anchor point from which to resolve the
    relative import to an absolute import.

    """
    level = 0
    if name.startswith('.'):
        if not package:
            msg = ("the 'package' argument is required to perform a relative "
                   "import for {!r}")
            raise TypeError(msg.format(name))
        for character in name:
            if character != '.':
                break
            level += 1
    return _bootstrap._gcd_import(name[level:], package, level)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 yeah ok. It sounds a bit more like a detail. Fine if I keep it like it is?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more transparent for the caller as they'd know which errors to handle without having to dive deeper into the code, but up to you as it's a detail as you said.

tests/shared/utils/test_common.py Outdated Show resolved Hide resolved
@rasabot rasabot merged commit 395f768 into master Jan 14, 2021
@rasabot rasabot deleted the fix-minor-issues-filter-sentry branch January 14, 2021 14:30
@m-vdb
Copy link
Collaborator Author

m-vdb commented Jan 14, 2021

finallyyyyyyyyyyy merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants