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

migrating node.js APM configuration #16219

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tlhunter
Copy link
Member

@tlhunter tlhunter commented Dec 14, 2022

What does this PR do?

  • previously, the node.js APM team treated this page as the source of truth for configuration docs
  • that information is being brought over to this repository and removed from the other website

Motivation

  • hoping to make things more consistent
  • maintaining docs in two places is silly

Preview

https://docs-staging.datadoghq.com/tlhunter/migrate-nodejs-config/tracing/trace_collection/library_config/nodejs

Additional Notes


Reviewer checklist

  • Review the changed files.
  • Review the URLs listed in the Preview section.
  • Check images for PII
  • Review any mentions of "Contact Datadog support" for internal support documentation.

@tlhunter tlhunter requested a review from a team as a code owner Dec 14, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 14, 2022

Copy link
Contributor

@Qard Qard left a comment

Lots of great improvements! 🎉

Whether to enable the tracer.
: **Configuration**: N/A<br>
**Default**: `true`<br>
Whether to enable dd-trace. Setting this to `false` will disable all features of the library.
Copy link
Contributor

@Qard Qard Dec 15, 2022

Choose a reason for hiding this comment

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

We should probably have a discussion about if it's confusing to document both DD_TRACE_ENABLED and DD_TRACING_ENABLED. Can we make a different env var for the global disable to make the distinction clearer and deprecate this one?

Copy link
Contributor

@kayayarai kayayarai Dec 15, 2022

Choose a reason for hiding this comment

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

I agree. I think an explanation of "this is different from DD_TRACE_ENABLED, which does X, whereas this does Y" would be a first step.

Copy link
Member Author

@tlhunter tlhunter Dec 16, 2022

Choose a reason for hiding this comment

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

I agree that its confusing. At first glance it seems to be a mistake on our part for offering both. @Qard @rochdev do either of you know how these two features differ?

@kayayarai kayayarai self-requested a review Dec 15, 2022
@kayayarai kayayarai self-assigned this Dec 15, 2022
Copy link
Contributor

@kayayarai kayayarai left a comment

Great PR! Left you some edit feedback. Let me know if you have questions about any of them, or when you need a re-review.

@tlhunter tlhunter force-pushed the tlhunter/migrate-nodejs-config branch from eaa8df3 to 1f4c332 Compare Dec 16, 2022
@tlhunter tlhunter force-pushed the tlhunter/migrate-nodejs-config branch from 1f4c332 to 7ee135b Compare Dec 16, 2022
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.

None yet

4 participants