Uploaded image for project: 'Couchbase .NET client library'
  1. Couchbase .NET client library
  2. NCBC-1461

Do not log failed N1QL prepare queries

    XMLWordPrintable

Details

    • Bug
    • Status: Resolved
    • Minor
    • Resolution: Fixed
    • None
    • 2.5.1
    • library

    Description

      When monitoring production environments, logging about specific N1QL queries that are failing would be very helpful.  Queries might be problematic due to bad statement syntax, targeting the wrong/missing bucket, missing index, inefficient index causing a timeout, etc.

      This log would only apply If the failure is related to the query itself (i.e. not just an HTTP communication error).  Logging the actual query text that failed would help with identifying bad query and locating it in code.

      The Warn log level is probably the most appropriate for this log.

       

      NOTE: I renamed the ticket from
      Log failed N1QL queries to assist with production monitoring
      to
      Do not log failed N1QL prepare queries

      Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

          Hi Brant Burnett

          I did discuss briefly with Matt last week and while we think this is a good idea, printing out the query statement could have security implications. We can discuss more.

          mike.goldsmith Michael Goldsmith added a comment - Hi Brant Burnett I did discuss briefly with Matt last week and while we think this is a good idea, printing out the query statement could have security implications. We can discuss more.

          Ahhh, that's a good point Michael Goldsmith , I didn't think about the security implications.  However, I would point out that you're already doing it when queries fail to prepare.  So that might be something to look at, too.

          Though now I'm going to contradict myself.  If using prepared queries, the portion that would affect security should be in parameters, which aren't part of the query test.  So maybe that's fine.

          btburnett3 Brant Burnett added a comment - Ahhh, that's a good point Michael Goldsmith , I didn't think about the security implications.  However, I would point out that you're already doing it when queries fail to prepare.  So that might be something to look at, too. Though now I'm going to contradict myself.  If using prepared queries, the portion that would affect security should be in parameters, which aren't part of the query test.  So maybe that's fine.

          I agree we should not be logging failed prepare statements either.

          We need a consistent and safe way of logging both failed ad-hoc and prepared queries.

          mike.goldsmith Michael Goldsmith added a comment - I agree we should not be logging failed prepare statements either. We need a consistent and safe way of logging both failed ad-hoc and prepared queries.

          After discussing with SDK team, we feel that the SDK should not be responsible for logging this information. All the details can be logged by the consuming application if it chooses to do so between the original request and the result.

          The commit I've added removes the logging for failed prepare statement so that we are consistent across SDKs.

          mike.goldsmith Michael Goldsmith added a comment - After discussing with SDK team, we feel that the SDK should not be responsible for logging this information. All the details can be logged by the consuming application if it chooses to do so between the original request and the result. The commit I've added removes the logging for failed prepare statement so that we are consistent across SDKs.

          People

            mike.goldsmith Michael Goldsmith
            btburnett3 Brant Burnett
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Gerrit Reviews

                There are no open Gerrit changes

                PagerDuty