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

Add HTTP plugin #157

Closed
mayurkale22 opened this issue Aug 2, 2019 · 4 comments · Fixed by #161
Closed

Add HTTP plugin #157

mayurkale22 opened this issue Aug 2, 2019 · 4 comments · Fixed by #161
Assignees
@OlivierAlbertini
Labels
feature-request
Milestone
Basic Plugins

Comments

@mayurkale22
Copy link
Member

mayurkale22 commented Aug 2, 2019 •
edited
Loading

Add opentelemetry-plugin-http to allow the user to automatically collect trace data. The package should be used by both server and client. This should be based on BasePlugin interface: https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts#L20

OpenCensus reference implementation: https://github.com/census-instrumentation/opencensus-node/tree/master/packages/opencensus-instrumentation-http
@mayurkale22 mayurkale22 added feature-request up-for-grabs Good for taking. Extra help will be provided by maintainers labels Aug 2, 2019
@OlivierAlbertini
Copy link
Member

OlivierAlbertini commented Aug 3, 2019 •
edited
Loading

This PR #146 should be merged before.
And Tracer (from node-tracer) should expose the new feature.

Related:

@OlivierAlbertini OlivierAlbertini mentioned this issue Aug 3, 2019
Closed
@vmarchaud
Copy link
Member

@OlivierAlbertini I fail to see why #146 need to be merged, it only add internal stuff to the ah propagation which will not be exposed anyway

@OlivierAlbertini
Copy link
Member

OlivierAlbertini commented Aug 4, 2019 •
edited
Loading

Hello @vmarchaud

For HTTP plugin, we need to wrap request and response object for event emitters. I thought that this PR added the propagation for event emitters.

EDIT: I see that bind method is already present. It's just matter of testing with the real implementation.

Since it's not mandatory, I can start the implementation. Thanks @vmarchaud.

@vmarchaud
Copy link
Member

vmarchaud commented Aug 4, 2019 •
edited
Loading

I was going to explain the bind method but you edited your comment ^^
Also note that the binding of event emitters for req/res is done to propagate the scope for future spans and not used in the actual patching

OlivierAlbertini added a commit to OlivierAlbertini/opentelemetry-js that referenced this issue Aug 4, 2019
@OlivierAlbertini
feat(plugin): add http plugin
e174d4f 
Closes open-telemetry#157

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
@OlivierAlbertini OlivierAlbertini mentioned this issue Aug 4, 2019
Merged
@mayurkale22 mayurkale22 removed the up-for-grabs Good for taking. Extra help will be provided by maintainers label Aug 5, 2019
OlivierAlbertini added a commit to OlivierAlbertini/opentelemetry-js that referenced this issue Aug 5, 2019
@OlivierAlbertini
feat(plugin): add http plugin
a99e08d 
Closes open-telemetry#157

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
OlivierAlbertini added a commit to OlivierAlbertini/opentelemetry-js that referenced this issue Aug 8, 2019
@OlivierAlbertini
feat(plugin): add http plugin
717d60a 
Closes open-telemetry#157

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
OlivierAlbertini added a commit to OlivierAlbertini/opentelemetry-js that referenced this issue Aug 8, 2019
@OlivierAlbertini
feat(plugin): add http plugin
38bc270 
Closes open-telemetry#157

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
OlivierAlbertini added a commit to OlivierAlbertini/opentelemetry-js that referenced this issue Aug 10, 2019
@OlivierAlbertini
feat(plugin): add http plugin
d36a011 
Closes open-telemetry#157

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
OlivierAlbertini added a commit to OlivierAlbertini/opentelemetry-js that referenced this issue Aug 16, 2019
@OlivierAlbertini
feat(plugin): add http plugin
0662839 
Closes open-telemetry#157

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
OlivierAlbertini added a commit to OlivierAlbertini/opentelemetry-js that referenced this issue Aug 21, 2019
@OlivierAlbertini
feat(plugin): add http plugin
182dd73 
Closes open-telemetry#157

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
@mayurkale22 mayurkale22 mentioned this issue Aug 23, 2019
Merged
OlivierAlbertini added a commit to OlivierAlbertini/opentelemetry-js that referenced this issue Aug 26, 2019
@OlivierAlbertini
feat(plugin): add http plugin
ab7ca98 
Closes open-telemetry#157

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
OlivierAlbertini added a commit to OlivierAlbertini/opentelemetry-js that referenced this issue Aug 28, 2019
@OlivierAlbertini
feat(plugin): add http plugin
04b5cc0 
Closes open-telemetry#157

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
OlivierAlbertini added a commit to OlivierAlbertini/opentelemetry-js that referenced this issue Aug 30, 2019
@OlivierAlbertini
feat(plugin): add http plugin
4b140a3 
Closes open-telemetry#157

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
mayurkale22 pushed a commit that referenced this issue Aug 30, 2019
@OlivierAlbertini @mayurkale22
feat(plugin): add http plugin (#161)
3f90bf9 
* 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>

* fix: add Flarna recommendations

fix: add Flarna recommendations
fix: tests
fix: OC bugs in OT only
test: add assertions
Allow options object as second argument

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

* refactor: simplify propagation usage

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

* fix: add Flarna recommandations

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

* refactor(test): use ReadableSpan

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

* feat: export class/enums for https module

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

* refactor: plugin.enable has logger param

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>

* fix: add license header, rename enum files and refactoring

refactor: make integration tests mandatory for ci only
remove duplicate tests
remove dead comments

Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
@dyladan
Merge pull request open-telemetry#157 from dynatrace-oss-contrib/merg…
5efad74 
…ed-dependabot-update

chore: bump codecov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@OlivierAlbertini OlivierAlbertini

Labels
feature-request
Projects
None yet
Milestone
Basic Plugins
Development

Successfully merging a pull request may close this issue.

feat(plugin): add http plugin OlivierAlbertini/opentelemetry-js
3 participants
@vmarchaud @OlivierAlbertini @mayurkale22