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

[plugin-http] Create a span builder #394

Closed
OlivierAlbertini opened this issue Oct 3, 2019 · 2 comments · Fixed by #643
Closed

[plugin-http] Create a span builder #394

OlivierAlbertini opened this issue Oct 3, 2019 · 2 comments · Fixed by #643
Labels
enhancement New feature or request
Milestone

Comments

@OlivierAlbertini
Copy link
Member

OlivierAlbertini commented Oct 3, 2019

Instead of building all attributes in the HttpPlugin class, ctor should take a SpanBuilder that add attributes to the span.

In this way,

  • HttpPlugin will be easier to read.
  • People who specializes this plugin could pass their spanBuilder in order to add attributes they want. (this argument would lead to the discussion of having the option to pass applyCustomAttributesOnSpan, so no need to have a try/catch and check if the callback exists)
  • Can be reused in the http2 plugin.
@OlivierAlbertini OlivierAlbertini added the enhancement New feature or request label Oct 3, 2019
@mayurkale22
Copy link
Member

Is this still relevant?

@OlivierAlbertini
Copy link
Member Author

Hi Mayur Kale,

I believe, it would be useful for http/2 plugin, also it's a suggestion in order to centralize how we build span for http in a general manner. It will be easier for mainteance. WDYT ?

OlivierAlbertini added a commit to VilledeMontreal/opentelemetry-js that referenced this issue Dec 21, 2019
@mayurkale22 mayurkale22 added this to the Alpha v0.3.2 milestone Dec 23, 2019
mayurkale22 added a commit that referenced this issue Jan 2, 2020
* feat(plugin-http): add/modify attributes

closes #373, #394

Signed-off-by: Olivier Albertini <[email protected]>

* fix: change remotePort to localPort

refactor: remove useless checks
test: add assertions

Signed-off-by: Olivier Albertini <[email protected]>

* test(plugin-https): sync with http plugin

Signed-off-by: Olivier Albertini <[email protected]>

Co-authored-by: Mayur Kale <[email protected]>
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants