Fix to DBM propagation - comment is injected before query is sent #2938

crysmags · 2023-03-24T23:32:05Z

What does this PR do?

Motivation

Plugin Checklist

Additional Notes

github-actions · 2023-03-24T23:32:59Z

Overall package size

Self size: 4.01 MB
Deduped: 59.61 MB
No deduping: 59.65 MB

Dependency sizes

name	version	self size	total size
@datadog/native-iast-taint-tracking	1.3.1	13.47 MB	13.47 MB
@datadog/pprof	2.1.0	12.31 MB	13.2 MB
@datadog/native-appsec	2.0.0	12.33 MB	12.34 MB
@datadog/native-metrics	1.5.0	7.1 MB	7.11 MB
protobufjs	7.1.2	2.76 MB	6.55 MB
@datadog/native-iast-rewriter	2.0.1	2.09 MB	2.1 MB
opentracing	0.14.7	194.81 kB	194.81 kB
@datadog/sketches-js	2.1.0	109.9 kB	109.9 kB
lodash.sortby	4.7.0	75.76 kB	75.76 kB
lru-cache	7.14.0	74.95 kB	74.95 kB
semver	5.7.1	61.58 kB	61.58 kB
ipaddr.js	2.0.1	59.52 kB	59.52 kB
ignore	5.2.0	48.87 kB	48.87 kB
import-in-the-middle	1.3.4	32.7 kB	37.17 kB
istanbul-lib-coverage	3.2.0	29.34 kB	29.34 kB
retry	0.10.1	27.44 kB	27.44 kB
lodash.uniq	4.5.0	25.01 kB	25.01 kB
limiter	1.1.5	23.17 kB	23.17 kB
lodash.kebabcase	4.1.1	17.75 kB	17.75 kB
lodash.pick	4.4.0	16.33 kB	16.33 kB
node-abort-controller	3.0.1	14.33 kB	14.33 kB
crypto-randomuuid	1.0.0	11.18 kB	11.18 kB
diagnostics_channel	1.1.0	7.07 kB	7.07 kB
path-to-regexp	0.1.7	6.78 kB	6.78 kB
koalas	1.0.2	6.47 kB	6.47 kB
methods	1.1.2	5.29 kB	5.29 kB
module-details-from-path	1.0.3	4.47 kB	4.47 kB

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

codecov · 2023-03-24T23:35:19Z

Codecov Report

Merging #2938 (b81660e) into master (79fc8ec) will decrease coverage by 0.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #2938      +/-   ##
==========================================
- Coverage   87.64%   87.64%   -0.01%     
==========================================
  Files         329      329              
  Lines       11747    11750       +3     
  Branches       33       33              
==========================================
+ Hits        10296    10298       +2     
- Misses       1451     1452       +1

Impacted Files	Coverage Δ
packages/datadog-instrumentations/src/pg.js	`84.09% <88.88%> (+1.16%)`	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pr-commenter · 2023-03-25T00:06:45Z

Benchmarks

Comparing candidate commit 318e6d9 in PR branch dbm_PG_Fix_crys with baseline commit cfaadf2 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 676 metrics, 32 unstable metrics.

Qard · 2023-03-26T20:38:26Z

packages/datadog-plugin-pg/test/index.spec.js

          }).then(done, done)

          client.query('SELECT $1::text as message', ['Hello World!'], (err, result) => {
            if (err) return done(err)
+            expect(seenTraceParent).to.be.true
+            net.Socket.prototype.write = originalWrite


This will not get restored if the assertion before it fails. This could cause other tests to fail incorrectly. It's generally best to avoid side effects potentially leaking out of tests. This could be resolved by putting the test in a describe with before and after to do the environment modifications, ensuring they always happen regardless of what happens in the test.

describe('something', () => { before(() => { // Do your environment-changing things... }) after(() => { // Ensure your environment changes are restored, even if the tests failed. }) it('should work', () => { // Your modified environment state will be present here. }) })

Got it, good point, will make the changes 👍

Hi @Qard I have added the tests to a beforeEach and and afterEach block.

…uery.apply is called

rochdev · 2023-04-04T02:41:46Z

packages/datadog-plugin-pg/test/index.spec.js

@@ -150,7 +148,6 @@ describe('Plugin', () => {
              expect(traces[0][0].meta).to.have.property(ERROR_MESSAGE, error.message)
              expect(traces[0][0].meta).to.have.property(ERROR_STACK, error.stack)
              expect(traces[0][0].meta).to.have.property('component', 'pg')
-              expect(traces[0][0].metrics).to.have.property('network.destination.port', 5432)


Any way to not remove these tests?

I am not sure how they were removed...

Added back 👍

) * adding query to async resource to ensure we add the comment before query.apply is called * changing full propagation test to check for comment before call to db

…sent (#2938)" This reverts commit 840f390.

* Update sql-injection-analyzer.js * cleanup

) * adding query to async resource to ensure we add the comment before query.apply is called * changing full propagation test to check for comment before call to db

* Update sql-injection-analyzer.js * cleanup

) * adding query to async resource to ensure we add the comment before query.apply is called * changing full propagation test to check for comment before call to db

* Update sql-injection-analyzer.js * cleanup

crysmags requested a review from a team as a code owner March 24, 2023 23:32

crysmags added the semver-minor label Mar 24, 2023

crysmags force-pushed the dbm_PG_Fix_crys branch from c165ffa to 5a4d84b Compare March 24, 2023 23:52

Qard reviewed Mar 26, 2023

View reviewed changes

crysmags added 3 commits April 3, 2023 14:13

adding retval to async resource to ensure we add the comment before q…

45fa6a5

…uery.apply is called

changing full propagation test to check for comment before call to db

d366b62

adding prototype.write to before and after block in test

8421ebc

crysmags force-pushed the dbm_PG_Fix_crys branch from 5a4d84b to 8421ebc Compare April 3, 2023 18:20

crysmags added 3 commits April 3, 2023 14:25

adding prototype.write to beforeEach and afterEach block in test

0ae4a32

adding prototype.write to beforeEach and afterEach block in test

237ae3a

adding prototype.write to beforeEach and afterEach block in test

0b7ac8a

tlhunter previously approved these changes Apr 3, 2023

View reviewed changes

Qard previously approved these changes Apr 3, 2023

View reviewed changes

rochdev reviewed Apr 4, 2023

View reviewed changes

adding prototype.write to beforeEach and afterEach block in test

318e6d9

crysmags dismissed stale reviews from Qard and tlhunter via 318e6d9 April 4, 2023 18:07

Merge branch 'master' into dbm_PG_Fix_crys

b81660e

rochdev approved these changes Apr 5, 2023

View reviewed changes

crysmags merged commit 840f390 into master Apr 5, 2023

This was referenced Apr 5, 2023

v2.30.0 #2964

Merged

v3.17.0 #2965

Merged

simon-id added a commit that referenced this pull request Apr 6, 2023

Revert "Fix to DBM propagation - comment is injected before query is …

dba8cd1

…sent (#2938)" This reverts commit 840f390.

simon-id mentioned this pull request Apr 6, 2023

Fix #2938 #2976

Merged

simon-id added a commit that referenced this pull request Apr 6, 2023

Fix #2938 (#2976)

bb7d3c4

* Update sql-injection-analyzer.js * cleanup

tlhunter pushed a commit that referenced this pull request Apr 6, 2023

Fix #2938 (#2976)

d9493b8

* Update sql-injection-analyzer.js * cleanup

tlhunter pushed a commit that referenced this pull request Apr 6, 2023

Fix #2938 (#2976)

3f98a43

* Update sql-injection-analyzer.js * cleanup

tlhunter pushed a commit that referenced this pull request Apr 6, 2023

Fix #2938 (#2976)

1919b2d

* Update sql-injection-analyzer.js * cleanup

tlhunter pushed a commit that referenced this pull request Apr 6, 2023

Fix #2938 (#2976)

5edef54

* Update sql-injection-analyzer.js * cleanup

tlhunter mentioned this pull request Apr 19, 2023

Postgres regression: spans no longer include query #3007

Closed

comp615 mentioned this pull request Apr 28, 2023

[fix: plugin-pg] Support Query config object again #3085

Closed

tlhunter deleted the dbm_PG_Fix_crys branch January 19, 2024 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to DBM propagation - comment is injected before query is sent #2938

Fix to DBM propagation - comment is injected before query is sent #2938

crysmags commented Mar 24, 2023

github-actions bot commented Mar 24, 2023 •

edited

Loading

codecov bot commented Mar 24, 2023 •

edited

Loading

pr-commenter bot commented Mar 25, 2023 •

edited

Loading

Qard Mar 26, 2023

crysmags Mar 27, 2023

crysmags Apr 3, 2023

rochdev Apr 4, 2023

crysmags Apr 4, 2023

crysmags Apr 4, 2023

crysmags Apr 4, 2023

Fix to DBM propagation - comment is injected before query is sent #2938

Fix to DBM propagation - comment is injected before query is sent #2938

Conversation

crysmags commented Mar 24, 2023

What does this PR do?

Motivation

Plugin Checklist

Additional Notes

github-actions bot commented Mar 24, 2023 • edited Loading

Overall package size

Dependency sizes

codecov bot commented Mar 24, 2023 • edited Loading

Codecov Report

pr-commenter bot commented Mar 25, 2023 • edited Loading

Benchmarks

Qard Mar 26, 2023

Choose a reason for hiding this comment

crysmags Mar 27, 2023

Choose a reason for hiding this comment

crysmags Apr 3, 2023

Choose a reason for hiding this comment

rochdev Apr 4, 2023

Choose a reason for hiding this comment

crysmags Apr 4, 2023

Choose a reason for hiding this comment

crysmags Apr 4, 2023

Choose a reason for hiding this comment

crysmags Apr 4, 2023

Choose a reason for hiding this comment

github-actions bot commented Mar 24, 2023 •

edited

Loading

codecov bot commented Mar 24, 2023 •

edited

Loading

pr-commenter bot commented Mar 25, 2023 •

edited

Loading