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

Automatic tracing of express #666

Closed
vmarchaud opened this issue Jan 5, 2020 · 7 comments
Closed

Automatic tracing of express #666

vmarchaud opened this issue Jan 5, 2020 · 7 comments
Assignees
Milestone

Comments

@vmarchaud
Copy link
Member

vmarchaud commented Jan 5, 2020

Is it applicable for Node or Browser or both
Node only

Do you expect this plugin to be commonly used
Weekly Downloads: 6M (https://www.npmjs.com/package/express)

What version of plugin are you interested in using
Versions: 4

Additional context

Implementation of datadog: https://github.com/DataDog/dd-trace-js/tree/master/packages/datadog-plugin-express

@vmarchaud vmarchaud added this to the Alpha v0.4 milestone Jan 5, 2020
@vmarchaud vmarchaud self-assigned this Jan 5, 2020
@dyladan
Copy link
Member

dyladan commented Jan 5, 2020

What do you think would be gained beyond the http information already available? Are we better off auto-instrumenting it or just exporting a middleware that uses the API?

@OlivierAlbertini
Copy link
Member

OlivierAlbertini commented Jan 5, 2020

What do you think would be gained beyond the http information already available?

You can get the route attribute.

I have already done an express plugin internally for OpenTelemetry and I don't see a real advantage to have it in this repo (I'm not against to have it too)... I did it because some packages like body-parser seems to clear the scope... So I bind the middleware after body-parser to recreate it if this happens. You can also create an error middleware where you can add the detailed error to the span...

I think the express plugin is more useful for customizing the span.

@dyladan
Copy link
Member

dyladan commented Jan 6, 2020

dropping scope is a big deal so that sounds like a good win for us. can you work with @vmarchaud since you already made an internal version?

@vmarchaud
Copy link
Member Author

@OlivierAlbertini I didn't had the issue with my OC plugin, that's weird.

@dyladan That was a common request back when i worked at @keymetrics to allow customers to aggregate by the route (since it's common to have an id in urls). Also useful to pinpoint if you have a sync middleware (like auth with jwt) taking a lot of time.

dropping scope is a big deal so that sounds like a good win for us

I'm not sure to understand since it was dropping the scope it should fail to create child spans afterward, i don't see why it's a good win ^^

@dyladan
Copy link
Member

dyladan commented Jan 6, 2020

@vmarchaud let me rephrase. If the scope is being dropped, and creating a plugin can prevent it from being dropped, then that sounds like a good win.

@vmarchaud
Copy link
Member Author

@dyladan Got it, indeed its important in that case.

@OlivierAlbertini
Copy link
Member

@OlivierAlbertini I didn't had the issue with my OC plugin, that's weird.

Currently we have Opencensus and OpenTelemetry in our environnement and we have the issue with OpenCensus "0.0.19" for sure and OpenTelemetry "0.1.1".

It doesn't happens every time (it's really hard to reproduce) and I don't know how the body parser works but having a middleware after have fixed the issue. I flagged those spans differently in order to see if there is a pattern...

@dyladan, yes I can work with @vmarchaud. I can make a draft PR and request a review (@vmarchaud ) but you can create a PR too since you made the issue and I can review it... As you want @vmarchaud, you decide ^^

vmarchaud added a commit to vmarchaud/opentelemetry-js that referenced this issue Jan 11, 2020
vmarchaud added a commit to vmarchaud/opentelemetry-js that referenced this issue Jan 19, 2020
vmarchaud added a commit to vmarchaud/opentelemetry-js that referenced this issue Jan 26, 2020
vmarchaud added a commit to vmarchaud/opentelemetry-js that referenced this issue Feb 1, 2020
@dyladan dyladan mentioned this issue Feb 12, 2020
46 tasks
@mayurkale22 mayurkale22 modified the milestones: Alpha v0.4, Beta Feb 12, 2020
vmarchaud added a commit to vmarchaud/opentelemetry-js that referenced this issue Feb 13, 2020
vmarchaud added a commit to vmarchaud/opentelemetry-js that referenced this issue Feb 20, 2020
dyladan pushed a commit that referenced this issue Feb 20, 2020
* feat: add express plugin #666

* feat: set http.route attribute on http server span if possible

* feat: add config to ignore express layers

* chore: add documentation about express layer store
@dyladan dyladan closed this as completed Feb 20, 2020
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
…pen-telemetry#818)

* fix hasOwnProperty check

* Revert "fix hasOwnProperty check"

This reverts commit 671021cb6e9732ef14ef50f69feabaa3d49d61a4.

* fix(opentelemetry/instrumentation-redis) hasOwnProperty check fixes duplicate call check

in my project this code was called a second time based on importing. However in the case it was called again the check would fail. By switching to hasOwnProperty it fixes this issue.

* adding tests and changing stream check to not crash

* fix(opentelemetry/instrumentation-redis) changing has stream check

This reverts commit 5a04acc4b9ce47713c0fc5eec18cdd5ad52ab70d.

* style: fix lint

Co-authored-by: Jonathan Campos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants