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.

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

chore: adding plugin-fetch and example #1121

Merged
merged 18 commits into from
Jun 17, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented May 28, 2020

Which problem is this PR solving?

Short description of the changes

  • Adds auto instrumentation for fetch in browser

@codecov
Copy link

codecov bot commented May 28, 2020

Codecov Report

Merging #1121 into master will increase coverage by 0.16%.
The diff coverage is 96.77%.

@@            Coverage Diff             @@
##           master    #1121      +/-   ##
==========================================
+ Coverage   92.16%   92.32%   +0.16%     
==========================================
  Files         120      122       +2     
  Lines        3409     3533     +124     
  Branches      696      714      +18     
==========================================
+ Hits         3142     3262     +120     
- Misses        267      271       +4     
Impacted Files Coverage Δ
packages/opentelemetry-web/src/types.ts 100.00% <ø> (ø)
...s/opentelemetry-plugin-xml-http-request/src/xhr.ts 76.66% <66.66%> (-2.65%) ⬇️
packages/opentelemetry-plugin-fetch/src/fetch.ts 96.42% <96.42%> (ø)
...telemetry-plugin-fetch/src/enums/AttributeNames.ts 100.00% <100.00%> (ø)
...kages/opentelemetry-web/src/StackContextManager.ts 97.05% <100.00%> (ø)
packages/opentelemetry-web/src/utils.ts 94.48% <100.00%> (+1.09%) ⬆️

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Haven't had time to look at the actual plugin implementation yet, but just a couple small first pass nits here.

Have wanted this plugin for a while so this is awesome thanks!

/**
* https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md
*/
export enum AttributeNames {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should have a package just for semantic convention constants and possibly even builder classes. Not really an issue for this PR, just a thought

Copy link
Member Author

Choose a reason for hiding this comment

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

they could be a part of core or maybe even an api

packages/opentelemetry-plugin-fetch/src/fetch.ts Outdated Show resolved Hide resolved
packages/opentelemetry-plugin-fetch/tsconfig.json Outdated Show resolved Hide resolved
@obecny obecny requested a review from dyladan May 28, 2020 19:06
Copy link
Member

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM w.r.t. getting the fetch plugin ability into opentelemetry

@obecny
Copy link
Member Author

obecny commented Jun 8, 2020

@open-telemetry/javascript-approvers @open-telemetry/javascript-maintainers
^^

Copy link
Member

@markwolff markwolff left a comment

Choose a reason for hiding this comment

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

Overall LGTM!

packages/opentelemetry-web/src/utils.ts Outdated Show resolved Hide resolved
@mayurkale22
Copy link
Member

Added a few minor comments in the first pass.

@obecny obecny requested a review from naseemkullah as a code owner June 9, 2020 12:11
@obecny obecny requested a review from mayurkale22 June 9, 2020 12:42
@dyladan dyladan added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Jun 16, 2020
@mayurkale22 mayurkale22 merged commit 0d939d2 into open-telemetry:master Jun 17, 2020
@obecny obecny deleted the plugin-fetch branch July 8, 2020 12:16
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
* chore: adding plugin-fetch and example

* chore: investigating failing test

* chore: chore fixing tests with better fetch mocking

* chore: addressing comments

* chore: lint

* chore: addressing comments

* chore: updating webpack-env

* chore: fixes after update for node types

* chore: addressing reviews

* chore: fixes after merge

* chore: updating version

Co-authored-by: Mayur Kale <[email protected]>
Co-authored-by: Daniel Dyla <[email protected]>
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
* chore: adding plugin-fetch and example

* chore: investigating failing test

* chore: chore fixing tests with better fetch mocking

* chore: addressing comments

* chore: lint

* chore: addressing comments

* chore: updating webpack-env

* chore: fixes after update for node types

* chore: addressing reviews

* chore: fixes after merge

* chore: updating version

Co-authored-by: Mayur Kale <[email protected]>
Co-authored-by: Daniel Dyla <[email protected]>
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin for fetch instrumentation
6 participants