fix(instrumentation-mysql2)!: instrumentation should not include values in db.statement #1857

macno · 2023-12-11T13:16:30Z

Which problem is this PR solving?

This PR addresses the bug in #1758 also following what emerged in open-telemetry/semantic-conventions#436

Short description of the changes

This is a breaking change

The DB statement will be included in the span only when placeholders are used. The rationale behind this choice is that if there are no placeholders, it is assumed that the query SQL may include sensitive data, and therefore, no DB statement is included in the telemetry.

codecov · 2023-12-11T15:54:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.81%. Comparing base (6158af0) to head (6f0a8cb).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1857      +/-   ##
==========================================
+ Coverage   90.75%   90.81%   +0.06%     
==========================================
  Files         169      169              
  Lines        8026     8035       +9     
  Branches     1635     1634       -1     
==========================================
+ Hits         7284     7297      +13     
+ Misses        742      738       -4

Files with missing lines	Coverage Δ
...etry-instrumentation-mysql2/src/instrumentation.ts	`100.00% <100.00%> (+5.33%)`	⬆️
.../opentelemetry-instrumentation-mysql2/src/utils.ts	`93.75% <100.00%> (+0.56%)`	⬆️

blumamir

Thanks for the fix!
Added few nit optional comments

plugins/node/opentelemetry-instrumentation-mysql2/src/utils.ts

plugins/node/opentelemetry-instrumentation-mysql2/src/instrumentation.ts

plugins/node/opentelemetry-instrumentation-mysql2/test/mysql2.test.ts

plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts

tolgap · 2024-04-19T10:36:49Z

I just found this PR. Its quite egregious to include values in logs. Opens up a lot of auditing issues. Would be great to have this! Seems like development halted on this though 😞

JamieDanielson · 2024-06-20T16:24:01Z

@macno is this something you are still working on? It seems this type of change will be desired, and we are wondering whether you are able to keep moving forward with this or if we should keep it as an open issue to pick up.

This would be part of updating/fixing #1552

macno · 2024-06-21T08:01:56Z

Sorry, I was working on the tests when I had to interrupt.
The code is OK, if some someone else want to pick it up from here that's great, I'm not sure if I can work on this in the near future

github-actions · 2024-08-21T06:36:03Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

macno · 2024-11-07T17:07:47Z

Hi @JamieDanielson I'll be at kubecon next week, and I'm considering to work on this during the Contribfest, WDYT?

trentm · 2024-11-07T23:27:04Z

I'm considering to work on this during the Contribfest, WDYT?

Sounds good. I'll be there too.

maryliag · 2024-11-13T00:05:05Z

For whoever decides to work on this, a few considerations:

we do have current semantic convention saying we should not collect the value if is not sanitized, but the prior versions didn't say that. I'm not sure which version was used when this was created, but we do have a warning that if was using 1.24 or prior and you make a change, you need to use the env variable OTEL_SEMCONV_STABILITY_OPT_IN for the new changes
I think this can still be treated as a bug fix
This will be a temporary solution. We have a WG focusing on database metrics, and one of the things we're focusing now is that we want to have values not sanitized as not being set by default, and give the option to set the value anyway (similar to what will be done here), but we want to do that we the config file currently being developed. This way all databases will be consistent. So this can still be worked on, but it will be temporary and eventually replaced.

trentm · 2024-11-16T00:15:17Z

Docker compose snippet:

  mysql:
    image: mysql:5.7
    # No ARM64 image layer. See https://stackoverflow.com/a/65592942
    platform: linux/x86_64
    environment:
      MYSQL_ALLOW_EMPTY_PASSWORD: 1
    ports:
      - "3306:3306"
    volumes:
      - nodemysqldata:/var/lib/mysql
    healthcheck:
      test: ["CMD", "mysql" ,"-h", "mysql", "-P", "3306", "-u", "root", "-e", "SELECT 1"]
      interval: 1s
      timeout: 10s
      retries: 30

macno · 2024-11-16T00:53:27Z

to run the tests on arm64, spin up a container manually

services:
  mysql:
    image: mysql:5.7
    platform: linux/amd64
    command:
      - --log_output=TABLE
      - --general_log=ON
    environment:
      MYSQL_ROOT_PASSWORD: rootpw
      MYSQL_USER: otel
      MYSQL_PASSWORD: secret
      MYSQL_DATABASE: test_db
    ports:
      - "33306:3306"

and then run: RUN_MYSQL_TESTS=1 npm run test

github-actions · 2024-11-18T06:39:20Z

This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature.
Are you familiar with this package? Consider becoming a component owner.

trentm · 2024-11-18T22:11:30Z

I'll sponsor this to avoid the autoclose for unmaintained packages.

trentm

The plan for this PR has changed. Please update the PR description to describe the current plan.
Given this is a breaking change, please change the PR title to be fix(instrumentation-mysql2)!: instrumentation should not include values in db.statement. The exclamation point indicates a breaking change for the release process.
Note: instrumentation-mysql dealt with this same issue (including values in 'db.statement') in fix(mysql): add enhancedDatabaseReporting to mysql #1337 In that PR a enhancedDatabaseReporting boolean config option was added to get the old behaviour back. I think this PR should not add this config option and instead just make the breaking change and wait for common DB handling to be specified by the DB SIG (as Marylia mentioned above).

plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts

plugins/node/opentelemetry-instrumentation-mysql2/src/utils.ts

trentm · 2024-11-19T00:05:19Z

plugins/node/opentelemetry-instrumentation-mysql2/src/instrumentation.ts

+        if (
+          arguments.length === 1 ||
+          (arguments.length === 2 && typeof arguments[1] !== 'function')
+        ) {


This is an interesting fix that could be mentioned in the PR description. IIUC, before this change, the following usage would result in an unended span:

const sql = 'SELECT 1+? as solution'; const query = connection.query(sql, [2]);

not sure it was a bug, I had to add it since some existing tests were failing. Anyway, looking at it after your comment, I rewrote the statement to simplify it.

trentm · 2024-11-19T00:12:37Z

@macno This looks great, just a few small requested changes.

macno · 2024-11-19T07:48:18Z

@macno This looks great, just a few small requested changes.

thank you @trentm ! I'm going to take care of your suggestions, much appreciated

trentm · 2024-11-20T00:20:18Z

@macno a side-node: If you are able to avoid force-pushing to a feature branch after there are reviews, that would be helpful. A force push removes the earlier commit shas, so I am not able to see the changes since my last review, for example.

It is okay to have a bunch of commits on a feature branch. They will all be squashed into a single commit when the PR is merged.

trentm

Looking good. I have some suggestions, but they don't need to block.

trentm · 2024-11-20T00:45:20Z

plugins/node/opentelemetry-instrumentation-mysql2/src/instrumentation.ts

          span.end();
        });

-        if (arguments.length === 1) {
+        if (typeof arguments[arguments.length - 1] !== 'function') {


This is okay, but isn't exactly the logic that mysql2 uses to determine if a callback function is passed in. For example, I think the following usage would work with the library, but break the instrumentation:

const sql = 'SELECT 1+1 as solution'; const query = connection.query(sql, cb, null);

Granted that's spectacularly unlikely to be used by a user. :)

I added a fixup that should address the point you raised

trentm · 2024-11-20T01:27:35Z

plugins/node/opentelemetry-instrumentation-mysql2/src/instrumentation.ts

+          if (isWrapped(ConnectionPrototype.query)) {
+            this._unwrap(ConnectionPrototype, 'query');
+          }
+          if (isWrapped(ConnectionPrototype.execute)) {
+            this._unwrap(ConnectionPrototype, 'execute');
+          }


I'm curious, was this added because of your added "initialization" test below? Was that resulting in logger('no original to unwrap to -- has ' + name + ' already been unwrapped?') console errors?

yes, correct

trentm · 2024-11-20T01:33:50Z

plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts

          });
        }
      });
    });
  });

+  describe('initialization', () => {


Why the addition of this test? Did you hit some specific issue?

I think this is not testing what you think it is testing.

With both instrumentation and instrumentation2 being enabled: the first one wraps, then the second one unwrap and wraps. Then when .disable() is called for both, the first one unwraps, and the second one is a no-op.

When running just this "initialization" test and with some added logging to show the value of isWrapped(mysqlTypes.Connection.prototype.query) before and after each enable/disable call, it looks like this:

% RUN_MYSQL_TESTS=1 npm test > @opentelemetry/[email protected] test > nyc mocha 'test/**/*.test.ts' mysql2 initialization XXX instr2.enable() isWrapped? false XXX instr2.enabled isWrapped? true XXX instr.enable() isWrapped? true XXX instr.enabled isWrapped? true ✔ should not wrap more than once XXX instr.disable() isWrapped? true XXX instr.disabled isWrapped? false XXX instr2.disable() isWrapped? false XXX instr2.disabled isWrapped? false 1 passing (17ms)

My diff for that test run is here: https://gist.github.com/trentm/d85145848dce0f45b8c440a6f655a078

We are actually considering dropping Instrumentation#disable() functionality altogether. See [instrumentation] consider dropping Instrumentation#disable() opentelemetry-js#5154 if you are curious.

My preference would be to not add this test.

Honestly, I added it to reach 100% functions and lines coverage and see the branches not covered.
At the moment the only branch not covered is this one https://github.com/macno/opentelemetry-js-contrib/blob/issue-1758-mysql2-instrumentations-values-in-db.statement/plugins/node/opentelemetry-instrumentation-mysql2/src/instrumentation.ts#L80 which I have no idea how to test..

…db.statement span attribute (open-telemetry#1758)

…ues to db.statement span attribute (open-telemetry#1758)

macno requested a review from a team December 11, 2023 13:16

github-actions bot assigned blumamir, dyladan, haddasbronfman, legendecas and pichlermarc Dec 11, 2023

github-actions bot requested review from blumamir, dyladan, haddasbronfman, legendecas and pichlermarc December 11, 2023 13:16

macno changed the title ~~Fix mysql2 instrumentation always includes values in db.statement~~ fix(instrumentation-mysql2) instrumentation should not include values in db.statement Dec 11, 2023

macno changed the title ~~fix(instrumentation-mysql2) instrumentation should not include values in db.statement~~ fix(instrumentation-mysql2): instrumentation should not include values in db.statement Dec 11, 2023

macno force-pushed the issue-1758-mysql2-instrumentations-values-in-db.statement branch from 7c101a0 to 6d7aedf Compare December 11, 2023 15:45

macno force-pushed the issue-1758-mysql2-instrumentations-values-in-db.statement branch 3 times, most recently from 8abdce1 to 4fa33f0 Compare December 12, 2023 04:23

blumamir approved these changes Dec 31, 2023

View reviewed changes

JamieDanielson added the information-requested label Jun 20, 2024

github-actions bot added stale and removed stale labels Aug 21, 2024

macno force-pushed the issue-1758-mysql2-instrumentations-values-in-db.statement branch from 4fa33f0 to 77ee46c Compare November 17, 2024 19:24

macno requested a review from a team as a code owner November 17, 2024 19:24

github-actions bot added pkg:instrumentation-mysql2 pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found. labels Nov 17, 2024

github-actions bot added the pkg-status:unmaintained:autoclose-scheduled label Nov 18, 2024

macno force-pushed the issue-1758-mysql2-instrumentations-values-in-db.statement branch 2 times, most recently from 497024f to a2fe940 Compare November 18, 2024 07:50

trentm added the has:sponsor This package or feature has a sponsor that has volunteered to review PRs and respond to questions label Nov 18, 2024

trentm requested changes Nov 19, 2024

View reviewed changes

macno force-pushed the issue-1758-mysql2-instrumentations-values-in-db.statement branch from a2fe940 to d5cd88f Compare November 19, 2024 13:46

macno requested a review from trentm November 19, 2024 13:51

macno changed the title ~~fix(instrumentation-mysql2): instrumentation should not include values in db.statement~~ fix(instrumentation-mysql2)!: instrumentation should not include values in db.statement Nov 19, 2024

trentm approved these changes Nov 20, 2024

View reviewed changes

fix(instrumentation-mysql2)!: do not include parameterized values to …

ee6f9b6

…db.statement span attribute (open-telemetry#1758)

macno force-pushed the issue-1758-mysql2-instrumentations-values-in-db.statement branch from d5cd88f to ee6f9b6 Compare December 7, 2024 15:55

fixup! fix(instrumentation-mysql2)!: do not include parameterized val…

6cf79ef

…ues to db.statement span attribute (open-telemetry#1758)

macno force-pushed the issue-1758-mysql2-instrumentations-values-in-db.statement branch from 6f0a8cb to 6cf79ef Compare December 7, 2024 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(instrumentation-mysql2)!: instrumentation should not include values in db.statement #1857

fix(instrumentation-mysql2)!: instrumentation should not include values in db.statement #1857

macno commented Dec 11, 2023 •

edited

Loading

codecov bot commented Dec 11, 2023 •

edited

Loading

blumamir left a comment

tolgap commented Apr 19, 2024

JamieDanielson commented Jun 20, 2024

macno commented Jun 21, 2024

github-actions bot commented Aug 21, 2024

macno commented Nov 7, 2024

trentm commented Nov 7, 2024

maryliag commented Nov 13, 2024

trentm commented Nov 16, 2024

macno commented Nov 16, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024

trentm commented Nov 18, 2024

trentm left a comment

trentm Nov 19, 2024

macno Nov 19, 2024

trentm commented Nov 19, 2024

macno commented Nov 19, 2024

trentm commented Nov 20, 2024

trentm left a comment

trentm Nov 20, 2024

macno Dec 7, 2024

trentm Nov 20, 2024

macno Dec 7, 2024

trentm Nov 20, 2024

macno Dec 7, 2024

fix(instrumentation-mysql2)!: instrumentation should not include values in db.statement #1857

Are you sure you want to change the base?

fix(instrumentation-mysql2)!: instrumentation should not include values in db.statement #1857

Conversation

macno commented Dec 11, 2023 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Dec 11, 2023 • edited Loading

Codecov Report

blumamir left a comment

Choose a reason for hiding this comment

tolgap commented Apr 19, 2024

JamieDanielson commented Jun 20, 2024

macno commented Jun 21, 2024

github-actions bot commented Aug 21, 2024

macno commented Nov 7, 2024

trentm commented Nov 7, 2024

maryliag commented Nov 13, 2024

trentm commented Nov 16, 2024

macno commented Nov 16, 2024 • edited Loading

github-actions bot commented Nov 18, 2024

trentm commented Nov 18, 2024

trentm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trentm commented Nov 19, 2024

macno commented Nov 19, 2024

trentm commented Nov 20, 2024

trentm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macno commented Dec 11, 2023 •

edited

Loading

codecov bot commented Dec 11, 2023 •

edited

Loading

macno commented Nov 16, 2024 •

edited

Loading