Skip to content

add missing shared config for clientIpHeader#5473

Merged
wconti27 merged 13 commits intomasterfrom
ida613/plugin-manager-patch
Apr 2, 2025
Merged

add missing shared config for clientIpHeader#5473
wconti27 merged 13 commits intomasterfrom
ida613/plugin-manager-patch

Conversation

@ida613
Copy link
Copy Markdown
Collaborator

@ida613 ida613 commented Mar 24, 2025

What does this PR do?

clientIpHeader was missing from shared config, meaning that users couldn't remap the client IP header to the http.client_ip span tag. This PR adds the shared config and adds test cases for the config itself along with test cases within the http plugin to ensure functionality works as intended.

Motivation

Plugin Checklist

Additional Notes

@ida613 ida613 requested a review from a team as a code owner March 24, 2025 18:53
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 24, 2025

Overall package size

Self size: 9.19 MB
Deduped: 101.5 MB
No deduping: 102.01 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.5.0 | 29.83 MB | 29.83 MB | | @datadog/native-appsec | 8.5.1 | 19.26 MB | 19.27 MB | | @datadog/native-iast-taint-tracking | 3.3.0 | 13.77 MB | 13.78 MB | | @datadog/pprof | 5.6.0 | 9.79 MB | 10.16 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.4.0 | 2.77 MB | 5.42 MB | | @datadog/wasm-js-rewriter | 3.1.0 | 2.37 MB | 2.52 MB | | @datadog/native-metrics | 3.1.0 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.13.1 | 117.64 kB | 839.26 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | dc-polyfill | 0.1.6 | 24.56 kB | 24.56 kB | | shell-quote | 1.8.2 | 23.54 kB | 23.54 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 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
Copy link
Copy Markdown

codecov Bot commented Mar 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.18%. Comparing base (8e21dd1) to head (302a77d).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5473      +/-   ##
==========================================
- Coverage   79.25%   79.18%   -0.07%     
==========================================
  Files         513      512       -1     
  Lines       23230    23158      -72     
==========================================
- Hits        18410    18338      -72     
  Misses       4820     4820              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@datadog-datadog-prod-us1
Copy link
Copy Markdown

datadog-datadog-prod-us1 Bot commented Mar 24, 2025

Datadog Report

Branch report: ida613/plugin-manager-patch
Commit report: 20adf19
Test service: dd-trace-js-integration-tests

✅ 0 Failed, 928 Passed, 0 Skipped, 7m 16.45s Total Time

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Mar 24, 2025

Benchmarks

Benchmark execution time: 2025-04-02 00:17:55

Comparing candidate commit 302a77d in PR branch ida613/plugin-manager-patch with baseline commit 8e21dd1 in branch master.

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

Comment thread packages/dd-trace/test/plugin_manager.spec.js Outdated
@ida613 ida613 changed the title add missing shared config WIP: add missing shared config Mar 31, 2025
@ida613 ida613 marked this pull request as draft March 31, 2025 13:48
@wconti27 wconti27 self-assigned this Mar 31, 2025
@wconti27 wconti27 marked this pull request as ready for review March 31, 2025 17:03
@wconti27 wconti27 requested a review from a team as a code owner March 31, 2025 17:19
@wconti27 wconti27 changed the title WIP: add missing shared config add missing shared config for clientIpHeader Mar 31, 2025
Comment thread packages/datadog-plugin-http/test/server.spec.js Outdated
@wconti27 wconti27 requested a review from rochdev March 31, 2025 20:26
Comment thread packages/datadog-plugin-koa/test/index.spec.js Outdated
@wconti27 wconti27 enabled auto-merge (squash) April 2, 2025 00:18
})

it('should add custom client ip tag to the span when it is configured', () => {
req.headers['X-Forwad-For'] = '8.8.8.8'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the misspelling intended here ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure why you're changing the koa plugin tests ? ipHeader is not a koa or web framework thing. It's used directly in the web.js file, so the http plugin. Why is it tested here ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the PR was merged despite your comment. I agree it probably makes more sense to have an http test for this, but since all web frameworks inherit the http span anyway I guess any integration works. Since it's now merged, I personally am not too worried, but if you really think it should be moved to http that can be done as a follow-up PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's alright. It would have just been better to have a comment to explain this to the future maintainer of this file

@wconti27 wconti27 merged commit 5dc76ab into master Apr 2, 2025
427 checks passed
@wconti27 wconti27 deleted the ida613/plugin-manager-patch branch April 2, 2025 14:13
@wconti27 wconti27 mentioned this pull request Apr 8, 2025
wconti27 pushed a commit that referenced this pull request Apr 8, 2025
* add missing shared config for clientIpHeader
wconti27 pushed a commit that referenced this pull request Apr 9, 2025
* add missing shared config for clientIpHeader
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants