Hey
I've implemented this in my code.
I can see that traceparent is added to each query, unfortunately though the traces are not being linked up to my application traces. The query insights "end to end" view only shows the query breakdown without any of the application traces.
Perhaps tracespan is missing?
Any ideas how to debug this?

Hi,

First, I would thank you for open sourcing this library and releasing features around SQL performance and analysis, which is from my point of view, this is very important 🔥.

I would like to ask/propose SQLCommenter to support a really famous and important library of the JVM world named JOOQ.
As a Google Developer Expert on Google Cloud and Kotlin, I see a lot of interest on this library, to perform better queries and be able to optimize/analyse requests made to our SQL engine.

I know SQLCommenter is still young, but I open this issue to allow people to do a 👍 if they think this should be a good idea to support this too.

If you are open to PR, I'll be glad to propose something if you want.

Thanks

Hi,

Can ASP.NET Core be added to the roadmap please, since you already have a pretty darn good SDK for .NET and an awesome integration with ASP.NET.

Opentelemetry has released a stable 1.0 so I recommend adding a version specifier ~= 1.0. There may be some small breaking changes since then to resolve.

It would be great to be able to add extra/custom/generic key/value in the django middleware.

The usage could be something like that:

SQLCOMMENTER_WITH_EXTRA_VALUES = 'my.get_extra_values'
# or
SQLCOMMENTER_WITH_EXTRA_VALUES = my.get_extra_values

def get_extra_values(request):
    # ... using `request`
    return {'my_custom_key': 'my_custom_value'}

I'm willing to work on a PR if the idea is accepted.

    if execute.__self__.__class__ is CursorDebugWrapper:
AttributeError: 'functools.partial' object has no attribute '__self__

I am using Python3.6 with Django2.2.13.

I also tested a simple code that reproduces this error with a trivial class.

import functools
class ClassA:
    def foo(self):
        pass

functools.partial(ClassA.foo).__self__  # functools.partial(ClassA.foo, ClassA()).__self__ also fails

Hi,

I'm experimenting with SQL Commenter for Java (using version 2.0.1). We're using Spring Boot 2.7.0 and Hibernate (the version that ships with spring boot) and I've followed the instructions here. I.e. I've added:

@Configuration
MyConfigurer implements WebMvcConfigurer {
	
   @Bean
    public SpringSQLCommenterInterceptor sqlInterceptor() {
         return new SpringSQLCommenterInterceptor();
    }
 
    @Override
    public void addInterceptors(InterceptorRegistry registry) {
        registry.addInterceptor(sqlInterceptor());
    }
  
}

and added

properties.setProperty("hibernate.session_factory.statement_inspector", SCHibernate.class.getName());

to our Hibernate configuration.

Then I've started MySQL (5.6) in Docker and enabled logging:

 SET GLOBAL general_log = 'ON';

If I run SHOW VARIABLES LIKE "general_log%"; I see this:

+------------------+---------------------------------+
| Variable_name    | Value                           |
+------------------+---------------------------------+
| general_log      | ON                              |
| general_log_file | /var/lib/mysql/fb28ada36c57.log |
+------------------+---------------------------------+

When I log into the container and tail the log I see a lot SQL queries but I don't see any comments that I expect to see with sql-commenter.

What am I doing wrong?

I can't find com.google.cloud:sqlcommenter-java anywhere, but I do find com.google.cloud:sqlcommenter. Is this the same package?

In our application, a lot of our database load comes from scheduled celery tasks. They still use the Django ORM to access the database. Since they're coming from celery however, they don't go through the Middleware chain, so the queries aren't getting annotated by the google.cloud.sqlcommenter.django.middleware.SqlCommenter middleware.

Are there any suggestions on how to integrate sqlcommenter in with celery?

Latest version on pypi is 2.0.0 which was released Aug 18, 2021
https://pypi.org/project/google-cloud-sqlcommenter/#history

However there have been a few important python commits since that date including Add compatability for ; in sqlalchemy,psycopg2 & django (#123)
https://github.com/google/sqlcommenter/commits/master/python/sqlcommenter-python/google/cloud/sqlcommenter/django/middleware.py

R2DBC is another database connector but a reactive one

https://google.github.io/sqlcommenter/python/flask/ references a class FlaskMiddleware but it does not seem to exist.

@adamegyed @aabmass While reading through the specification of the sqlcommenter, specifically the Comment Escaping section. It states that:

If a comment already exists within a SQL statement, we MUST NOT mutate that statement.

The above statement is clear for the scenarios when we have properly formed comment such as:

SELECT * from FOO -- this is the comment
SELECT * from FOO /* this is the comment */

But the spec does not state what happens when we have malformed comments, such as:

SELECT * from FOO /* this is the comment

SELECT * from FOO *//* this is the comment 

Looking at the implementation of sqlcommenter in the provided languages, there seem to be discrepancies. Please do correct me if that is wrong.

Java

  private Boolean hasSQLComment(String stmt) {
    if (stmt == null || stmt.isEmpty()) {
      return false;
    }

    // Perhaps we now have the closing comment or not but that doesn't matter
    // as the SQL should be reported as having an opening comment regardless
    // of it is in properly escaped or not.
    return stmt.contains("--") || stmt.contains("/*");
  }

In this implementation as soon as we see that there is a opening comment tag, we return the SQL statement as it is.

References:

1. Test cases

NodeJS

exports.hasComment = (sql) => {

    if (!sql)
        return false;

    // See https://docs.oracle.com/cd/B12037_01/server.101/b10759/sql_elements006.htm
    // for how to detect comments.
    const indexOpeningDashDashComment = sql.indexOf('--');
    if (indexOpeningDashDashComment >= 0)
        return true;

    const indexOpeningSlashComment = sql.indexOf('/*');
    if (indexOpeningSlashComment < 0)
        return false;

    // Check if it is a well formed comment.
    const indexClosingSlashComment = sql.indexOf('*/');
    
    /* c8 ignore next */
    return indexOpeningSlashComment < indexClosingSlashComment;
}

In this implementation we return false if there is a malformed comments which is totally opposite to what the implementation of java does.. That means, we do append the comment created by the library to the SQL statement.

References:

1. Test cases

Python and Ruby

I was unable to find the place where, we check if the SQL statement has any comments or not. Does this mean that the implementation in these languages, we append the comment created by library irrespective of if there is a comment present in the statement. I just had a cursory look in the library, so please do correct me if I'm wrong.

Keeping in consideration the above thing, the question would be:

What to do when we have a malformed comment in the SQL statement, do we ignore the statement and return it as it is OR do we append the comment to the SQL statement?

Hello

The FastAPI SQLCommenterMiddleware currently breaks FastAPI's openapi feature (see the log message at the end).
This is because this route uses a starlette.routing.Route (FastAPI is built on top of Starlette), which does not have the "route" info used in the code.

I think the easiest fix here would be to return child_scope.get("route") instead, so that if we find a matching child_scope but it lacks the info we're looking for the middleware simply returns no route info rather than crash.

Happy to create a merge request if this solution is OK with you.

Possible implementation of a currently failing test:

# in tests/fastapi/test.py
def test_get_docs_does_not_throw(client):  
    resp = client.get(app.docs_url)        
    assert resp.status_code == 200         

{
  "severity": "ERROR",
  "name": "uvicorn.error",
  "message": "Exception in ASGI application\n",
  "module": "httptools_impl",
  "process": 1626484,
  "processName": "MainProcess",
  "pathname": "<..>/lib/python3.8/site-packages/uvicorn/protocols/http/httptools_impl.py",
  "lineno": 378,
  "labels": {
    "app_name": "profile_api",
    "version": "1.14.0+query_insights.1"
  },
  "serviceContext": {
    "service": "profile_api",
    "version": "1.14.0+query_insights.1"
  },
  "stack_trace": "Traceback (most recent call last):	
    File \"<..>/lib/python3.8/site-packages/uvicorn/protocols/http/httptools_impl.py\", line 375, in run_asgi	
      result = await app(self.scope, self.receive, self.send)	
    File \"<..>/lib/python3.8/site-packages/uvicorn/middleware/proxy_headers.py\", line 75, in __call__	
      return await self.app(scope, receive, send)	
    File \"<..>/lib/python3.8/site-packages/uvicorn/middleware/message_logger.py\", line 82, in __call__	
      raise exc from None	
    File \"<..>/lib/python3.8/site-packages/uvicorn/middleware/message_logger.py\", line 78, in __call__	
      await self.app(scope, inner_receive, inner_send)	
    File \"<..>/lib/python3.8/site-packages/fastapi/applications.py\", line 269, in __call__	
      await super().__call__(scope, receive, send)	
    File \"<..>/lib/python3.8/site-packages/starlette/applications.py\", line 124, in __call__	
      await self.middleware_stack(scope, receive, send)	
    File \"<..>/lib/python3.8/site-packages/starlette/middleware/errors.py\", line 184, in __call__	
      raise exc	
    File \"<..>/lib/python3.8/site-packages/starlette/middleware/errors.py\", line 162, in __call__	
      await self.app(scope, receive, _send)	
    File \"<..>/lib/python3.8/site-packages/opentelemetry/instrumentation/asgi/__init__.py\", line 459, in __call__	
      await self.app(scope, otel_receive, otel_send)	
    File \"<..>/lib/python3.8/site-packages/google/cloud/sqlcommenter/fastapi.py\", line 69, in __call__	
      info = _get_fastapi_info(fastapi_app, scope)	
    File \"<..>/lib/python3.8/site-packages/google/cloud/sqlcommenter/fastapi.py\", line 84, in _get_fastapi_info	
      route = _get_fastapi_route(fastapi_app, scope)	
    File \"<..>/lib/python3.8/site-packages/google/cloud/sqlcommenter/fastapi.py\", line 98, in _get_fastapi_route	
      return child_scope[\"route\"]	
        KeyError: 'route'",
  "timestamp": "2022-05-27T08:23:55.549322+00:00"
}

Following https://packaging.python.org/guides/packaging-namespace-packages/#pkg-resources-style-namespace-packages

Would it be feasible to support attribute values being specified via a query's parameter bindings?

Idea

I'd like to support prepared statements like the following (and stay within the sqlcommeter spec):

LOG:  execute my-demo-stmt: SELECT * FROM FOO WHERE ID = $1 /*application='app-1', traceparent=$2*/
DETAIL:  parameters: $1 = 1234, $2 = '00-5bd66ef5095369c7b0d1f8f4bd33716a-c532cb4098ac3dd2-01'

This would necessitate an extension to sqlcommeter spec. Something like:

FIELD      ::=  <SERIALIZED_KEY>=<VALUE>
VALUE      ::=  <SERIALIZED_VALUE> | <PARAM_NUM>
PARAM_NUM  ::=  $[0-9]+

Motivation

My objective is to enable the performance benefits of prepared statements, whilst still being able to use a unique per-query traceparent.

In Postgres, it is technically possible to prepare and execute a statement like the example above. (The $2 parameter is simply unused by the query planner, but still appears in the statement log.) I am exploring adding this support to DataDog/dd-trace-go's SqlCommeter.

Would this feature in the spec be generally useful?

The code has been published under https://www.npmjs.com/package/@google-wombot/sqlcommenter and we'll need to explicit put that in our README instructions.

This effects both sequelize and knex as they use the same mechanism. The middleware sets the current request on the global Knex object:

And then uses this request in the wrapped query function to get route information

However, if the middleware runs again for a different concurrent request to a different route before the actual handler does the querying, it will overwrite Knex.__req__ with the new request. This can happen if the handler yields the event loop before it queries.

E.g. request comes for /foo and while it awaits something(), a request comes for /bar causing the middleware to overwrite __req__. Then /foo runs knex.select() and the query is annotated with the wrong request:

app.get('/foo', async (req, res) => {
  await something();
  const records = await knex.select(...);
  res.json(records);
});

app.get('/bar', async (req, res) => {
  const records = await knex.select(...);
  res.json(records);
});

Currently, only the queries on the default database are instrumented by sqlcommenter.

The django middleware uses django.db.connection, but when using multiple databases in django django.db.connections (with an extra s) should be iterated on (possibly re-implementing execute_wrapper to handle all connections in one wrapper).

I have a sqlcommenter project for .NET that has been running in production for 1 year called sqlcommenterdotnet if you would be interested in merging/forking?

So I've been investigating the bug I'm having in this isssue: #211

I was checking the way the query is sent to the database and realized that the current go implementation doesnt follow the specification.
The way the current implementation adds the comments is as such:
/*action=ListUsers,route=router,traceparent=00-ef43f0b3e8c3bf30186961dXXXX-YYYY-01*/

However, the specification states that all values must be single quoted

So I added single quotes to all the values that are being sent to the server and suddenly everything started working, so now I'm sending this instead (notice the single quotes on each parameter):
/*action='ListUsers',route='router',traceparent='00-ef43f0b3e8c3bf30186961dXXXX-YYYY-01'*/

This worked and solved the issue.

Based on the nodejs implementation it appears that all values need to be single quoted https://github.com/google/sqlcommenter/blob/master/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/index.js#L95

Attaching the PR to fix it below

There are applications using spring hibernate which don't use persistence.xml and also don't manage EntityManager using @configuration.
Some applications define their spring hibernate properties in application.yml.
There should be documentation to help those users.

I can submit a PR :)

There's certain comments that are necessary to include in a SQL statement. The use case I have in mind in Postgres hints 1.

  /*+
    HashJoin(a b)
    SeqScan(a)
  */
 EXPLAIN SELECT *
    FROM pgbench_branches b
    JOIN pgbench_accounts a ON b.bid = a.bid
   ORDER BY a.aid;

The spec states:

If a comment already exists within a SQL statement, we MUST NOT mutate that statement.

That means we can never use SQLCommenter with pg hint comments.

Proposal: modify spec to optionally allow affixing a comment. Alternately, narrow the spec to "if a comment already exists as the last token in a sql statement". When parsing choose the last comment if there's more than one.

Several otel packages need to be updated:
- "@opentelemetry/api": "^1.3.0",
- "@opentelemetry/core": "^1.8.0",
- "@opentelemetry/sdk-trace-node": "^1.9.0"

Notes:

• @opentelemetry/node was renamed to @opentelemetry/sdk-trace-node in v0.24.0
• HttptraceContextPropagator was renamed to W3CTraceContextPropagator in @opentelemetry/core in v0.26.0

index.js (for both knex and sequelize integration) requires util.js which requires @opencensus/propagation-tracecontext.

However, this dep is only in devDependencies atm. OpenCensus or OpenTelemetry are providers for tracestate/traceparent, so the dependency should be optional and only crash if the user passes tracestate/traceparent in their config.

Repro:

$ npm init -y
$ npm install @google-cloud/sqlcommenter-knex
$ node -i "require('@google-cloud/sqlcommenter-knex')"
internal/modules/cjs/loader.js:968
  throw err;
  ^

Error: Cannot find module '/usr/local/google/home/aaronabbott/repo/sqlcommenter/nodejs/sqlcommenter-nodejs/samples/express-opentelemetry/require('@google-cloud/sqlcommenter-knex')'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:965:15)
    at Function.Module._load (internal/modules/cjs/loader.js:841:27)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

Seems the package is actually in google.cloud.sqlcommenter but pages like https://google.github.io/sqlcommenter/python/flask/ and https://google.github.io/sqlcommenter/python/sqlalchemy/ import from sqlcommenter.

As seen in the tests: psycopg2-sqlcommenter-test, it seems comments are added after a ;.

These comments will not be part of the same query.

It is really common that the query is so long that the ending of the query will be trimmed in the logs.
If we can allow users to configure the location of comment to insert, that will make the library better.

I noticed a discrepancy how we go about URL encoding.

• Python and PHP implementations escape url encoding with %. This is mentioned nowhere in the specification.
• The Go and Javascript do a simple URL encode.
• The Java implementation also does not prefix but seems to double URL encode and reverse the order.

This results in three different results for the same traceparent example value of congo=t61rcWkgMzE,rojo=00f067aa0ba902b7:

Consulting the spec to look for answers, but also creates more questions:

Meta characters such as ' should be escaped with a slash .

The way this is written implies multiple characters, but only one is given?

1. URL encode the value e.g. given /param first, that SHOULD become %2Fparam%20first
2. Escape meta-characters within the raw value; a single quote ' becomes '

This seems to make the Go/Javascript implementations correct. What threw me off initially as my language of choice (Ruby) already encoded ' as %27 so the second point seemed superfluous. Checking the other implementations, I notice that Javascript encodeURIComponent does not do the same as Ruby and leaves the ' intact... but it also begs the questions why this workaround and not have it URL encoded as well?

What I'd like to see:

• A list of known URL encoding edge cases and their correct encoding form(s). There is already some listed in https://google.github.io/sqlcommenter/spec/#key-value-format but it contains a copy-paste error on the second row. I think the traceparent example and something like foo'bar are useful additions.
• The unit tests and readme files for implementations in this repository all using at least this list of cases.

Did a quick review on PHP Sqlcommenter integration test(#119) and approving it (to make it possible to have checks to before anyone can commit any code that breaks the library).

But to close this bug, we need to:

• Do a thorough review again and fix any issues due to that.
• Verify issues with git workflows added here.

Spring Boot 3 has been released with Spring 6.
However, it seems like sqlcommenter only supports Spring 5.

Would it be possible to support Spring 6/Spring Boot 3?

Sqlcommenter currently uses org.springframework.web.servlet.handler.HandlerInterceptorAdapter which is deprecated in Spring 5.3 and has been removed in Spring 6.

Aside from that, Spring moved from the javax to the jakarta root package due to the migration to Jakarta EE.

Proposal :
Add support for Spring gRPC Requests in sql commenter java.

Hello. Any plans to incorporate PHP and Laravel ORM? It would be much appreciated.

Currently all of the NodeJS sqlcommenter packages restrict the allowed comment entries to the DB driver, trace info, and the http route being served. I also want to specify the entry application so that I can tell where the query is coming from (especially in contexts that aren't serving http traffic!)

Are the allowed keys are being restricted on purpose? If so, would you accept a PR that allowed a static value for application to be specified at initialization time?

The Laravel/PHP package relies on version 0.9.0 of the OpenTelemetry package. Unfortunately, OpenTelemetry decided to unpublish all versions of the package prior to the 1.0.0.beta version. Running composer update in any project with this package installed will fail until the Laravel package is updated to support newer versions of OpenTelemetry

OpenTelemetry relies on node.js async hooks context in async calls. More details here:

This package provides async-hooks based context manager which is used internally by OpenTelemetry plugins to propagate specific context between function calls and async operations. It only targets NodeJS since async-hooks is only available there.

Unfortunately, this doesn't work with "thenables" which is knex.js uses (see open-telemetry/opentelemetry-js#940, knex/knex#3550) for node versions <14.5.0 (see nodejs/node#22360).

Thankfully, there is an existing plugin that solves this has solved this context issue for knex, which sqlcommenter-knex can base a solution on: https://github.com/myrotvorets/opentelemetry-plugin-knex/blob/bd5f2d918eeaa86c257f5e433772ec57b4834582/lib/knex.ts#L115

I have verified that this is working in newer node versions without any code changes.

Looking at the Key Value Format section, I'm a little confused

First, the second example seems to be off, the value serialize value + final don't match the initial value

Then in the 3rd example:

Shouldn't the name'' key has its quote escaped to name\'\'? And where does the second equal (=) in the Final form comes from?

Thank you.

Hi,

this is a great library.
Unfortunately we are mostly using Typeorm (https://typeorm.io/) which is not supported yet.
I think this is one of the most important node.js ORMs currently.

If anyone is interested, i can try to help out as much as i can.

Thanks

Would you be open to optionally supporting JSON as the format instead of the custom key-value format currently being used ?
Since the current format is so close to json anyway and given high quality implementation of json parsers in practically all platforms (and some native support in many major languages), it seems that using JSON would bring some benefits without significant downsides.

Reward Size in USD

5000 USD

Feature Proposal

I can add support for Php

Acceptance Criteria

Go to Mintycode to approve the Feature Proposal.

We need to figure out how to properly generate HTML from the website itself that'll then be rendered properly by the content retrieved from the gh-pages here.

@smg727 you have access to the sqlcommenter-website that I created and it can be run locally
by

hugo serve

or to generate HTML that'll be found in the /public directory

hugo

Though the LICENSE files in sqlcommenter-sequelize and sqlcommenter-knex are for Apache 2.0, package.json in both packages specifies "ISC".

• https://github.com/google/sqlcommenter/blob/master/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-sequelize/package.json#L40
• https://github.com/google/sqlcommenter/blob/master/nodejs/sqlcommenter-nodejs/packages/sqlcommenter-knex/package.json#L41