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

Expose sfqid attribute in snowflake exception messages #2201

Closed
swettk opened this issue Mar 13, 2020 · 5 comments · Fixed by #2358
Closed

Expose sfqid attribute in snowflake exception messages #2201

swettk opened this issue Mar 13, 2020 · 5 comments · Fixed by #2358
Labels
enhancement New feature or request good_first_issue Straightforward + self-contained changes, good for new contributors! snowflake

Comments

@swettk
Copy link

swettk commented Mar 13, 2020

When contacting snowflake support they always start the conversation with can you provide a query id. Exposing this id in all cases would be useful when contacting snowflake support.

@drewbanin drewbanin added good_first_issue Straightforward + self-contained changes, good for new contributors! snowflake enhancement New feature or request labels Mar 18, 2020
@drewbanin
Copy link
Contributor

Thanks @swettk - good idea! I think this should be straightforward to implement if anyone wants to give it a try :)

@manesioz
Copy link

Hey! Great project btw. If you need any help I'd love to take a stab at it.
After briefly looking over the source code, it seems like sfqid is already exposed/parsable, so not sure if I am looking in the wrong section (dbt/plugins/snowflake/dbt/adapters/snowflake/connections.py) or if it is not exposed as explicitly as you'd like.

What I mean is we have something like this:

    @contextmanager
    def exception_handler(self, sql):
        try:
            yield
        except snowflake.connector.errors.ProgrammingError as e:
            msg = str(e)

            logger.debug('Snowflake error: {}'.format(msg))

            if 'Empty SQL statement' in msg:
                logger.debug("got empty sql statement, moving on")
            elif 'This session does not have a current database' in msg:
                self.release()
                raise FailedToConnectException(
                    ('{}\n\nThis error sometimes occurs when invalid '
                     'credentials are provided, or when your default role '
                     'does not have access to use the specified database. '
                     'Please double check your profile and try again.')
                    .format(msg))
            else:
                self.release()
                raise DatabaseException(msg)
        except Exception as e:
            logger.debug("Error running SQL: {}", sql)
            logger.debug("Rolling back transaction.")
            self.release()
            if isinstance(e, RuntimeException):
                raise
            raise RuntimeException(str(e)) from e

And according to the docs, we can access the sfqid like:

cur = con.cursor()
try:
    cur.execute("SELECT * FROM testtable")
except snowflake.connector.errors.ProgrammingError as e:
    # default error message
    print(e)
    # customer error message
    print('Error {0} ({1}): {2} ({3})'.format(e.errno, e.sqlstate, e.msg, e.sfqid)) #note the e.sfqid

So would you want to explicitly log e.sfqid, or is the current method of exposing e itself sufficient? Unless there are instances where even that is not exposed?

@drewbanin
Copy link
Contributor

drewbanin commented Mar 22, 2020

Hey @manesioz - i think that's exactly the right place for this kind of logic! I think that we might not want to add the sfqid to stdout, but we can totally log it in the debug-level logs when errors occur.

I'd try something like:

@contextmanager
    def exception_handler(self, sql):
        try:
            yield
        except snowflake.connector.errors.ProgrammingError as e:
            msg = str(e)

            logger.debug(f'Snowflake error: {msg}')
            logger.debug(f'sfqid: {e.sfqid}')

Or similar. If you're up to it, it would be awesome if you could send through a PR! Let me know what you think :)

@ChristianKohlberg
Copy link
Contributor

If its okay im taking a shot at this issue to get me ramped up on the PR procedure.

I assume that the one-liner proposed by @drewbanin is sufficient to resolve the issue.

@drewbanin
Copy link
Contributor

closed by #2358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good_first_issue Straightforward + self-contained changes, good for new contributors! snowflake
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants