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

feat: Add parentSpanContext to Span and ReadableSpan #5422

Open
wants to merge 15 commits into
base: v1.x
Choose a base branch
from

Conversation

JacksonWeber
Copy link
Contributor

@JacksonWeber JacksonWeber commented Feb 5, 2025

Which problem is this PR solving?

This PR adds the parentSpanContext to the Span and ReadableSpan in order to adhere to the OTel spec.

Targets 1.x OTel, and will be accompanied by a follow-up PR against main with the breaking change of removing parentSpanId.

Fixes #5345

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Existing tests have been updated to include checks for the new parentSpanContext values.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (v1.x@ac8641a). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             v1.x    #5422   +/-   ##
=======================================
  Coverage        ?   94.66%           
=======================================
  Files           ?      322           
  Lines           ?     8061           
  Branches        ?     1633           
=======================================
  Hits            ?     7631           
  Misses          ?      430           
  Partials        ?        0           
Files with missing lines Coverage Δ
...kages/otlp-transformer/src/trace/internal-types.ts 100.00% <ø> (ø)
packages/opentelemetry-sdk-trace-base/src/Span.ts 97.63% <100.00%> (ø)
...ackages/opentelemetry-sdk-trace-base/src/Tracer.ts 98.57% <ø> (ø)

@JacksonWeber JacksonWeber marked this pull request as ready for review February 5, 2025 16:58
@JacksonWeber JacksonWeber requested a review from a team as a code owner February 5, 2025 16:58
@pichlermarc pichlermarc added the target:next-major-release This PR targets the next major release (`next` branch) label Feb 6, 2025
@pichlermarc pichlermarc added this to the OpenTelemetry SDK 2.0 milestone Feb 6, 2025
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

We should drop the parentSpanId, but otherwise looks good 🙂

experimental/CHANGELOG.md Outdated Show resolved Hide resolved
@JacksonWeber
Copy link
Contributor Author

JacksonWeber commented Feb 6, 2025

We should drop the parentSpanId, but otherwise looks good 🙂

Thanks for the review! I'll update the description of the PR, but I discussed with Daniel in the JS SIG - the idea behind this PR is to target the 1.x branch with the non-breaking change of adding the parentSpanContext and I'll create a follow-up PR to target main with the breaking change of removing parentSpanId. The purpose being to get unblocked sooner on having access to the parentSpanContext before the release of the 2.x SDK.

@pichlermarc pichlermarc dismissed their stale review February 6, 2025 19:29

I did not check which branch this is targeting.

@pichlermarc
Copy link
Member

@JacksonWeber my bad, did not see that this was targeting v1.x.

@dyladan does that mean that we're going to release a new v1.x feature version?

@pichlermarc pichlermarc removed the target:next-major-release This PR targets the next major release (`next` branch) label Feb 6, 2025
@pichlermarc pichlermarc removed this from the OpenTelemetry SDK 2.0 milestone Feb 6, 2025
@JacksonWeber
Copy link
Contributor Author

@pichlermarc Looks like there might be a perms issue with the W3C integration tests running against v1.x branch.

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