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

v0.8.0 #430

Merged
merged 27 commits into from
Feb 6, 2019
Merged

v0.8.0 #430

merged 27 commits into from
Feb 6, 2019

Conversation

rochdev
Copy link
Member

@rochdev rochdev commented Feb 6, 2019

No description provided.

rochdev and others added 27 commits December 17, 2018 14:22
* add automatic log correlation of tracer identifiers for pino

* add missing plugins to the build
* Add protocol as a configuration option for the dd-trace agent URL.

This is useful for scenarios where a SSL-terminating load balancer sits in front of the datadog-agent

* Fix lint errors

* Switching from protocol override to url override per CR

* Fixing missing comma from previous commit

* Fix tests

* overrideUrl -> url
@rochdev rochdev merged commit c8c49dc into master Feb 6, 2019
} else {
ScopeManager = require('./scope/scope_manager')
ScopeManager = require('../scope/scope_manager')

Choose a reason for hiding this comment

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

Hi @rochdev, I'm curious about this line. Why do we need the real scope manager for the noop tracer? We're checking the performance with the tracer on and off, but I suspect the async hooks code is still on even with enable=false on the tracer...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this was because we considered it a cross-cutting concern that can still be enabled if the tracer is disabled. In the upcoming 0.9.0 version the scope manager was rewritten from scratch and is no longer enabled when the tracer is disabled.

It's possible to still disable it in 0.8.0 by using DD_CONTEXT_PROPAGATION=false but it can have unwanted side effects if you use our integrations since they expect the scope manager to be available.

Copy link
Member Author

Choose a reason for hiding this comment

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

In general an overhead of around 20% can be expected from the async_hooks module which should hopefully improve with new versions of Node and v8.

Choose a reason for hiding this comment

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

Got it. Do you think the new version of the scope manager, independent of Node and v8, will have a performance impact? I seem to remember that it was less heavyweight...

Copy link
Member Author

Choose a reason for hiding this comment

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

It will definitely help. The current scope manager keeps track of the entire context, along with every parent/children to try to know when all descendants have finished execution, which ended up significantly increasing the complexity with basically no gain.

The new scope manager is very simple in nature and does nothing other than storing/retrieving spans in an object-based map. Any remaining overhead would generally be caused from the interaction between async_hooks and Promise which is very slow.

The only way I know at the moment to mitigate this issue is to either use a Promise library instead of native promises, which should be several orders of magnitude faster (even without async_hooks). I'm also experimenting with a solution that is not based on async_hooks which could help in the future but it wouldn't support async/await as it's a native construct, so that would need to be transpiled.

Choose a reason for hiding this comment

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

👍

@rochdev rochdev deleted the v0.8.0 branch December 20, 2023 05:14
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

Successfully merging this pull request may close these issues.

3 participants