fix(ts): added filter into ts definition file#7368
Conversation
Overall package sizeSelf size: 4.58 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.6 | 81.92 kB | 813.08 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
pabloerhard
force-pushed
the
pabloerhard/fix-http-ts
branch
from
5dd600b to
dd35ac7
Compare
pabloerhard
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7368 +/- ##
==========================================
- Coverage 80.41% 80.41% -0.01%
==========================================
Files 732 732
Lines 31050 31072 +22
==========================================
+ Hits 24970 24985 +15
- Misses 6080 6087 +7 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-02-05 17:12:59 Comparing candidate commit d329dae in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 234 metrics, 26 unstable metrics. |
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
This makes sense! Could you also do the same for queryStringObfuscation and resourceRenamingEnabled where it applies and remove the web interface? We have to adjust the test in that case to ignore web.
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
Could we have a brief call for the final review? I believe a call makes it easier for me to know which options apply where without having to dig deep
| */ | ||
| resourceRenamingEnabled?: boolean; | ||
| } | ||
| interface web extends HttpServer {} |
There was a problem hiding this comment.
Should we mention this plugin at all? It is more of an internal implementation detail. I did add it because I did want to show the options and it can be used, while it does not seem to be very helpful in this form.
| /** | ||
| * Custom filter function used to decide whether a URL/path is allowed. | ||
| * When provided, this takes precedence over allowlist/blocklist configuration. | ||
| * If not provided, allowlist/blocklist logic will be used instead. | ||
| * | ||
| * @param urlOrPath - The URL or path to filter | ||
| * @returns true to instrument the request, false to skip it | ||
| */ | ||
| filter?: (urlOrPath: string) => boolean; |
There was a problem hiding this comment.
I think this belongs to HttpClient
There was a problem hiding this comment.
web extends HttpServer and web also uses filter
There was a problem hiding this comment.
unless we move forward with removing web completely then it does make sense to just leave it in HttpClient
| /** | ||
| * Whether (or how) to obfuscate querystring values in `http.url`. | ||
| * | ||
| * - `true`: obfuscate all values | ||
| * - `false`: disable obfuscation | ||
| * - `string`: regex string used to obfuscate matching values (empty string disables) | ||
| * - `RegExp`: regex used to obfuscate matching values | ||
| */ | ||
| queryStringObfuscation?: boolean | string | RegExp; |
There was a problem hiding this comment.
This also applies to Http2Server
There was a problem hiding this comment.
We seem to miss tests that show how this would be used as end user. Please add these
| /** | ||
| * Whether to enable resource renaming when the framework route is unavailable. | ||
| */ | ||
| resourceRenamingEnabled?: boolean; |
There was a problem hiding this comment.
This is also used in azure_functions
There was a problem hiding this comment.
I am unsure if this is the correct place for an end user. Could you please write a kind of end user test?
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
I am actually uncertain how these methods would be defined as user and if this is indeed correct or not. After looking into it I understand where they are used but not how they are defined as a user.
Thus, adding tests for that would be really great! Also with more than a single filter for different parts, if possible.
This comment has been minimized.
This comment has been minimized.
pabloerhard
force-pushed
the
pabloerhard/fix-http-ts
branch
from
98e19be to
445ced6
Compare
| const promise = agent.assertSomeTraces(traces => { | ||
| assert.strictEqual(traces[0][0].resource, 'GET') | ||
| }) |
There was a problem hiding this comment.
This is not an ideal test since we only verify that any trace matches and that is definitely the case
|
|
||
| beforeEach(() => { | ||
| config = { | ||
| server: false, |
There was a problem hiding this comment.
Instead, could we add another filter and check if they conflict? I am uncertain if it is possible to define different filters.
And if they do, we should document that / add a comment here as well.
pabloerhard
force-pushed
the
pabloerhard/fix-http-ts
branch
from
02c6323 to
d329dae
Compare
* added filter into ts definition file * moved queryStringObfuscation and resourceRenamingEnabled * Added e2e tests for filter config in http http2 and redis * added tests for resourceRenamingEnabled in http * added tests for queryStringObfuscation * added test to verify multiple filters don't conflict
* added filter into ts definition file * moved queryStringObfuscation and resourceRenamingEnabled * Added e2e tests for filter config in http http2 and redis * added tests for resourceRenamingEnabled in http * added tests for queryStringObfuscation * added test to verify multiple filters don't conflict
* added filter into ts definition file * moved queryStringObfuscation and resourceRenamingEnabled * Added e2e tests for filter config in http http2 and redis * added tests for resourceRenamingEnabled in http * added tests for queryStringObfuscation * added test to verify multiple filters don't conflict
* added filter into ts definition file * moved queryStringObfuscation and resourceRenamingEnabled * Added e2e tests for filter config in http http2 and redis * added tests for resourceRenamingEnabled in http * added tests for queryStringObfuscation * added test to verify multiple filters don't conflict
2 participants
Please make sure your changes are properly tested!
What does this PR do?
Added filter definition into index.d.ts, previously it was only defined for the web interface but it is used in multiple places, with out this TypeScript users get an error when trying to add the filter option.
Motivation
Additional Notes