refactor(instrumentation-fetch): replace instanceof Request checks#6250
refactor(instrumentation-fetch): replace instanceof Request checks#6250js62789 wants to merge 6 commits intoopen-telemetry:mainfrom
instanceof Request checks#6250Conversation
…with duck-typing helper
| namespace: '@opentelemetry/opentelemetry-instrumentation-fetch/utils', | ||
| }); | ||
|
|
||
| export function isRequest(value: unknown): value is Request { |
There was a problem hiding this comment.
We may want to document that we duck-type for Request in this instrumentation as a result of this PR just to make it clear what the risk (even if minimal) of false positives.
There was a problem hiding this comment.
I've added some inline documentation. Do you think this change warrants an addition to the README as well?
There was a problem hiding this comment.
What do you think about an early exit for the common case?
if (value instanceOf Request) return true;
|
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. |
|
@js62789 are you still planning to work on this? Please keep in mind that the CLA has to be signed before we can accept any contributions. |
|
Yes, sorry about that. I forgot that this was open. Let me get this up to date. |
Resolved conflicts in instrumentation-fetch by keeping isRequest() helper while incorporating upstream changes to header injection logic.
|
@js62789 After you resolve conflicts, request me again and I'll take another look. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6250 +/- ##
==========================================
- Coverage 95.75% 95.38% -0.38%
==========================================
Files 375 337 -38
Lines 12725 11214 -1511
Branches 3013 2647 -366
==========================================
- Hits 12185 10696 -1489
+ Misses 540 518 -22 🚀 New features to boost your workflow:
|
# Conflicts: # experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
|
I've resolved the merge conflicts @overbalance |
overbalance
left a comment
overbalance
left a comment
There was a problem hiding this comment.
I left a comment about naming but it looks good. Thanks! Please let us know if it doesn't resolve your issue
| FetchCustomAttributeFunction, | ||
| FetchInstrumentationConfig, | ||
| } from '../src'; | ||
| import { isRequest as isRequestLike } from '../src/utils'; |
There was a problem hiding this comment.
I would stick with one name throughout. (isRequestLike would be my choice)
|
Make sure to run |
|
@overbalance Our team was able to upgrade our implementation of DataDome's scripts so that we didn't need to include the one monkey-patching the global Request constructor. For that reason, this is no longer an issue for us. I'm happy to get this ready to merge if you think it's something that will be beneficial for others. What do you think? |
|
@js62789 If that's the case, I recommend closing this without merging. If someone needs this in the future, we can revisit. |
4 participants
Which problem is this PR solving?
Our team is using DataDome for bot detection. They have set us up with a custom Captcha script which overrides the global Request constructor. We're going to see if they can eliminate this code smell, but in the case where they can't, I believe it would be valuable to improve the Request detection with this duck-typing helper instead. This will prevent requests to incorrect URLs when a patched request instance is provided to
fetch().Short description of the changes
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
rpm run testChecklist: