Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(instrumentation-http): add options to ignore requests #2704

Merged
merged 8 commits into from
Jan 17, 2022
Merged

feat(instrumentation-http): add options to ignore requests #2704

merged 8 commits into from
Jan 17, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jan 5, 2022 •
edited
Loading

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

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • http
  • https

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link

codecov bot commented Jan 5, 2022 •
edited
Loading

Codecov Report

Merging #2704 (d2ffa60) into main (d315b0a) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           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
Impacted Files Coverage Δ
...es/opentelemetry-instrumentation-http/src/utils.ts 99.06% <0.00%> (-0.03%) ⬇️
...ges/opentelemetry-instrumentation-http/src/http.ts 95.12% <0.00%> (+0.21%) ⬆️

@dyladan
Copy link
Member

dyladan commented Jan 5, 2022

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?

@legendecas
Copy link
Member Author

@dyladan good idea. will do an update.

@legendecas legendecas marked this pull request as ready for review January 8, 2022 17:31
@legendecas legendecas requested a review from a team January 8, 2022 17:31
@vmarchaud vmarchaud added the enhancement New feature or request label Jan 8, 2022
blumamir
blumamir reviewed Jan 10, 2022
View reviewed changes
experimental/packages/opentelemetry-instrumentation-http/README.md Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-instrumentation-http/README.md Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-instrumentation-http/src/utils.ts Show resolved Hide resolved
experimental/packages/opentelemetry-instrumentation-http/README.md Outdated
@@ -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 |
Copy link
Member

Choose a reason for hiding this comment

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

now that we have a general hook, should we deprecate these options, and consider removing them on the next breaking change (1.0.0)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with deprecating it. But I don't have a strong feeling about it. What do other folks think about it?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with it too, its preferable to have one generic option over multiple specifics

Copy link
Member

Choose a reason for hiding this comment

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

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
@dyladan
Copy link
Member

dyladan commented Jan 12, 2022

The PR title doesn't match what the PR is doing anymore

@legendecas legendecas changed the title feat(instrumentation-http): add options to ignore headers feat(instrumentation-http): add options to ignore requests Jan 12, 2022
@vmarchaud vmarchaud requested review from blumamir and a team January 15, 2022 17:27
@vmarchaud
Copy link
Member

@legendecas There is a lint issue after merging 82e39c4

blumamir
blumamir approved these changes Jan 16, 2022
View reviewed changes
Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM
The only open issue is whether we want to mark the previous non-generic config options as deprecated in this PR, or merge it as is and deprecate the agreed options later in the future.

@legendecas
Copy link
Member Author

Updated with linter fix and documented the deprecation status of previous specialized ignore matches.

@vmarchaud vmarchaud merged commit 21fc8b5 into open-telemetry:main Jan 17, 2022
@legendecas legendecas deleted the instrument/http branch January 17, 2022 09:45
@alexandernanberg alexandernanberg mentioned this pull request Mar 16, 2022
Closed
@dyladan dyladan mentioned this pull request Mar 16, 2022
Merged
@pichlermarc pichlermarc mentioned this pull request Oct 22, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@naseemkullah naseemkullah naseemkullah approved these changes

@blumamir blumamir blumamir approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@legendecas @dyladan @vmarchaud @blumamir @naseemkullah