feat(instrumentation-http): add options to ignore requests #2704
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2704 +/- ##
=======================================
Coverage 93.03% 93.04%
=======================================
Files 155 155
Lines 5371 5378 +7
Branches 1131 1137 +6
=======================================
+ Hits 4997 5004 +7
Misses 374 374
|
|
Instead of adding a new config to ignore each individual thing, why don't we add a callback config that receives the request object and returns a decision? |
|
@dyladan good idea. will do an update. |
legendecas
force-pushed
the
instrument/http
branch
from
25cdbdd
to
97e5d64
Compare
experimental/packages/opentelemetry-instrumentation-http/README.md
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-instrumentation-http/README.md
Outdated
Show resolved
Hide resolved
| @@ -51,7 +51,9 @@ Http instrumentation has few options available to choose from. You can set the f | |||
| | [`startIncomingSpanHook`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L97) | `StartIncomingSpanCustomAttributeFunction` | Function for adding custom attributes before a span is started in incomingRequest | | |||
| | [`startOutgoingSpanHook`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L99) | `StartOutgoingSpanCustomAttributeFunction` | Function for adding custom attributes before a span is started in outgoingRequest | | |||
| | [`ignoreIncomingPaths`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L87) | `IgnoreMatcher[]` | Http instrumentation will not trace all incoming requests that match paths | | |||
There was a problem hiding this comment.
now that we have a general hook, should we deprecate these options, and consider removing them on the next breaking change (1.0.0)?
There was a problem hiding this comment.
I'm fine with deprecating it. But I don't have a strong feeling about it. What do other folks think about it?
There was a problem hiding this comment.
I'm fine with it too, its preferable to have one generic option over multiple specifics
There was a problem hiding this comment.
agreed, nice generic approach 👍 i did the same for pino-http: pinojs/pino-http#153
experimental/packages/opentelemetry-instrumentation-http/src/http.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts
Show resolved
Hide resolved
experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts
Show resolved
Hide resolved
|
The PR title doesn't match what the PR is doing anymore |
legendecas
changed the title
|
@legendecas There is a lint issue after merging 82e39c4 |
|
Updated with linter fix and documented the deprecation status of previous specialized ignore matches. |
Which problem is this PR solving?
Add options to ignore requests with a custom hook, alongside the option to ignore paths/URLs. This can be helpful when people would like to ignore bots scrapes, etc.
Type of change
How Has This Been Tested?
Checklist: