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

feat(plugin): add http plugin #161

Open
wants to merge 11 commits into
base: master
from

Conversation

@OlivierAlbertini
Copy link
Contributor

commented Aug 4, 2019

UPDATE:

  • Increase the coverage and add fixes related to tests, Since we don't have exporter feature yet, I created a ProxyTracer (only for tests) in order to get spans. Perhaps, once we get exporter feature, we could refactor in order to use it, that should be simpler overall

UPDATE 2:

  • use bind method

Closes #157
Signed-off-by: Olivier Albertini olivier.albertini@montreal.ca

Thanks in advance for your feedback!

Show resolved Hide resolved packages/opentelemetry-node-tracer/src/NodeTracer.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/package.json Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/package.json Outdated
Show resolved Hide resolved packages/opentelemetry-basic-tracer/src/BasicTracer.ts Outdated
@vmarchaud

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

@OlivierAlbertini If you want some help don't hesitate to ping me on gitter (specially about the scopeManager use). I also speak french if you want

@rochdev
Copy link
Member

left a comment

There seem to be quite a few public methods. Is this intended? How is the plugin meant to be used?

@@ -34,7 +34,7 @@ export class BasicTracer implements types.Tracer {
private readonly _binaryFormat: types.BinaryFormat;
private readonly _httpTextFormat: types.HttpTextFormat;
private readonly _sampler: types.Sampler;
private readonly _scopeManager: ScopeManager;
readonly scopeManager: ScopeManager;

This comment has been minimized.

Copy link
@rochdev

rochdev Aug 5, 2019

Member

If the scope manager is exposed, getCurrentSpan is no longer needed. The idea of getCurrentSpan is to not expose the scope manager. Otherwise the same functionality is duplicated.

Should this be accessible from a method, for example tracer.scope()?

This comment has been minimized.

Copy link
@OlivierAlbertini

OlivierAlbertini Aug 5, 2019

Author Contributor

good point

This comment has been minimized.

Copy link
@OlivierAlbertini

OlivierAlbertini Aug 5, 2019

Author Contributor

Initially, I created a public method wrapEmitter on the NodeTracer. In this way, only 2 methods (active and bind) are exposed...

I'm also agree with @vmarchaud argument (e174d4f#r310554815)

We should not have specific method for a object type there since the actual handling is done inside the ah-scope-manager.

But this argument will be valid also for wrapSpan and getCurrentSpan

@mayurkale22 do you have some insight on this ?

This comment has been minimized.

Copy link
@vmarchaud

vmarchaud Aug 6, 2019

Member

I think we should hide the scopeManager and expose the with(), active() and bind in the tracer, since the with is required by spec.

This comment has been minimized.

Copy link
@vmarchaud

vmarchaud Aug 14, 2019

Member

I've opened #201 that should fix the problem. You will only need to rebase

Show resolved Hide resolved packages/opentelemetry-plugin-http/src/types.ts
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
@OlivierAlbertini

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

There seem to be quite a few public methods. Is this intended? How is the plugin meant to be used?

Public methods are statics and is it like that in order to reuse those methods for other plugins http2 / https or for people that would like to make their own plugin.

I'm open to change to protected if you prefer.

I created a separate static class

Show resolved Hide resolved packages/opentelemetry-plugin-http/package.json Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
@mayurkale22

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

I think you should add .DS_Store in .gitignore.

@mayurkale22

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

This is great start 💯

Added a few comments (first iteration). Please fix the build and rebase with master.

@OlivierAlbertini OlivierAlbertini force-pushed the OlivierAlbertini:feature/http-plugin branch 2 times, most recently from 0b98274 to 284aaeb Aug 5, 2019

@mayurkale22

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

yarn run fix should fix most of the lint issues.

@OlivierAlbertini OlivierAlbertini force-pushed the OlivierAlbertini:feature/http-plugin branch from 0e2d03f to 272b4b8 Aug 6, 2019

@OlivierAlbertini

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

yarn run fix should fix most of the lint issues.

yes, I got an issue where one package was on the gts 1.0.0 version instead of 1.1.0...
There was a comma feature conflict.

EDIT: I don't know what going on with my mac but I have clearly an issue with gts...
yarn run fix doesn't do the same thing that it does on circlesci or docker (with the same version of gts/node).

I will check a bit more why the issue is on my machine but my workaround is to do:
docker run -v $(pwd):/usr/src/lib -w "/usr/src/lib/packages/opentelemetry-plugin-http" ae28164cf6da yarn run fix

@OlivierAlbertini OlivierAlbertini force-pushed the OlivierAlbertini:feature/http-plugin branch from 272b4b8 to 7e77c66 Aug 6, 2019

@mayurkale22

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

We are using nyc tool now, you should remove c8 dependency https://github.com/open-telemetry/opentelemetry-js/pull/161/files#diff-1a29b0926f59784792eb19ccaeffb380R48, looks like missed during rebase.

Also, I would suggest not to upgrade gts in this PR, to keep it small for code review. Feel free to open a separate PR for it.

@OlivierAlbertini OlivierAlbertini force-pushed the OlivierAlbertini:feature/http-plugin branch 4 times, most recently from bfbf65d to bec4e2c Aug 6, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Aug 6, 2019

Codecov Report

Merging #161 into master will increase coverage by 3.76%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #161      +/-   ##
==========================================
+ Coverage   93.59%   97.36%   +3.76%     
==========================================
  Files          50       49       -1     
  Lines        1578     1517      -61     
  Branches       99      100       +1     
==========================================
  Hits         1477     1477              
+ Misses        101       40      -61
Impacted Files Coverage Δ
...ackages/opentelemetry-core/src/trace/NoopTracer.ts 93.75% <0%> (-6.25%) ⬇️
...ages/opentelemetry-basic-tracer/src/BasicTracer.ts 88% <0%> (-7.66%) ⬇️
...ges/opentelemetry-core/src/trace/TracerDelegate.ts 91.3% <0%> (-8.7%) ⬇️

@OlivierAlbertini OlivierAlbertini force-pushed the OlivierAlbertini:feature/http-plugin branch from bec4e2c to 710a491 Aug 6, 2019

@OlivierAlbertini

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Hi @rochdev

I added this issue #165

In order to avoid checking naming convention during PR review. Please feel free to comment and add your requirements.

@bg451
Copy link
Contributor

left a comment

Overall looks good to me, nothing raises any flags. That being said I'll defer to others with more experience monkey patching the http lib.

Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/utils.ts Outdated
status.message = error.message;

span.setStatus(status);
span.end();

This comment has been minimized.

Copy link
@bg451

bg451 Aug 6, 2019

Contributor

Should this be responsible for ending the span? For example, here it looks like there's a possibility for the same span to be ended multiple times (can you get both an error and end event for the response?), once if there's an error and then a guaranteed finish when the response ends.

This comment has been minimized.

Copy link
@OlivierAlbertini

OlivierAlbertini Aug 8, 2019

Author Contributor

Hi @bg451

can you get both an error and end event for the response?

the answer is no, node v8 to 12.

once if there's an error and then a guaranteed finish when the response ends.

Not sure to understand but basically if there is an error event, there is no res event.

@mayurkale22 mayurkale22 removed the request for review from justindsmith Aug 7, 2019

@OlivierAlbertini OlivierAlbertini force-pushed the OlivierAlbertini:feature/http-plugin branch 11 times, most recently from 6452b9c to a8519f0 Aug 8, 2019

Show resolved Hide resolved packages/opentelemetry-plugin-http/package.json Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/package.json Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/package.json Outdated
*/

// Should we export this to the types package in order to expose all values ?
export enum Format {

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Aug 10, 2019

Contributor

I am not sure if we need extra enum for the format, WDYT?

This comment has been minimized.

Copy link
@OlivierAlbertini

OlivierAlbertini Aug 11, 2019

Author Contributor

From my point of view, I prefer this instead of hardcoded the value but let me know. I'm open to change.

Show resolved Hide resolved packages/opentelemetry-plugin-http/src/models/spanFactory.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-plugin-http/src/http.ts Outdated
Show resolved Hide resolved packages/opentelemetry-basic-tracer/src/BasicTracer.ts Outdated
}

span
.setAttributes({

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Aug 10, 2019

Contributor

Could you please combine this with above span.setAttributes() line no 309?

This comment has been minimized.

Copy link
@OlivierAlbertini

OlivierAlbertini Aug 11, 2019

Author Contributor

Done! I also call setAttributes only once. Instead of each time... I think it's better. Let me know If you prefer the previous version.

OlivierAlbertini added some commits Aug 4, 2019

feat(plugin): add http plugin
Closes #157

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
fix: integrate vmarchaud recommandations
Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
fix: wip revert - gts fix issue on all packages
Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
fix: integrate mayurkale22 recommendations
Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
ci: gts fix for ci build
Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
refactor: add mayurkale22 recommendations
Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
fix: integrate bg451 recommendations
test: increase coverage
fix: attributes requirements from the spec.

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
fix: add missing header and revert scopeManager to private field
test: rename some tests
fix: copy/paste tests

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
fix: add tests and improve attributes from spec
fix parentSpanId instead of parentId from rebase
add workaround with got and node12+ (real http call)
improve args passed to function (url, options, cb)

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
fix: add mayurkale22 recommendations
Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
fix: rebase and remove/replace wrapEmitter to bind
Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

@OlivierAlbertini OlivierAlbertini force-pushed the OlivierAlbertini:feature/http-plugin branch from 2e4d0ef to e20f606 Aug 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.