-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Update propagation page for Node.js #4722
Conversation
Signed-off-by: svrnm <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just some suggestions.
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
Co-authored-by: Trent Mick <[email protected]>
/fix:all |
@open-telemetry/javascript-approvers please take a look! |
You triggered fix:all action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/9648516390 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, TY. See inline comments for feedback on the shortcode.I'll leave others carefully review the JS page changes.
propagate trace context across services for you. Only in rare cases you will | ||
need to propagate context manually. | ||
|
||
{{ if $lang -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have conditional content based on $lang
? The prose in the "then" clause below seems suitable in all cases.
If we don't need the "lang" parameter to the shortcode, I'd suggest that we drop it for now, and reintroduce it later if/when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The page this is referencing is /docs/concepts/context-propagation, so that would be self-referencing
Co-authored-by: Patrice Chalin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Co-authored-by: Trent Mick <[email protected]>
/fix:all |
You triggered fix:all action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/9790310929 |
Addresses #3688, this is a detailed updated on context propagation, that when applied can be replicated across languages.
Preview: https://deploy-preview-4722--opentelemetry.netlify.app/docs/languages/js/propagation/