Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

Add TraceState implementation to API #147

Merged
merged 8 commits into from
Jan 19, 2022

Conversation

srikanthccv
Copy link
Member

Follow up based on 29/12 call, TraceState implementation should be part of API for various reasons (Should be able to use in creating in custom propagator only by taking dependency on @opentelemetry/api , library instrumentation, specification). There is already an interface TraceState exported so introducing the factory function createTraceState that returns new TraceState implementation.

It is worth noting that there are other components that are supposed to be in api/sdk but are part of opentelemetry-core package but TraceState is something I needed at the moment.

@srikanthccv srikanthccv requested a review from a team December 31, 2021 20:56
@codecov
Copy link

codecov bot commented Dec 31, 2021

Codecov Report

Merging #147 (1b81bce) into main (aa65fc6) will increase coverage by 0.52%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #147      +/-   ##
==========================================
+ Coverage   94.67%   95.19%   +0.52%     
==========================================
  Files          42       45       +3     
  Lines         582      645      +63     
  Branches       94      102       +8     
==========================================
+ Hits          551      614      +63     
  Misses         31       31              
Impacted Files Coverage Δ
src/index.ts 100.00% <100.00%> (ø)
src/trace/internal/tracestate-impl.ts 100.00% <100.00%> (ø)
src/trace/internal/tracestate-validators.ts 100.00% <100.00%> (ø)
src/trace/internal/utils.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa65fc6...1b81bce. Read the comment docs.

@Flarna Flarna added the enhancement New feature or request label Jan 1, 2022
@dyladan
Copy link
Member

dyladan commented Jan 3, 2022

Is this the exact implementation from core or have you made any notable changes?

It is worth noting that there are other components that are supposed to be in api/sdk but are part of opentelemetry-core package but TraceState is something I needed at the moment.

Can you create issues for those components at least so we have something to track?

@srikanthccv
Copy link
Member Author

Is this the exact implementation from core or have you made any notable changes?

The only new change introduced in this PR createTraceState since there is already exported interface with TraceState name. Everything else is copied over from core package.

Can you create issues for those components at least so we have something to track?

Sure, will do that.

@Flarna
Copy link
Member

Flarna commented Jan 4, 2022

Are there already plans how to handle this move to API in @opentelemetry/core and other related packages?

  • @opentelemetry/core is GA so removing this class there would be breaking. Redirecting won't work as core exposes the constructor not the factory method which is not accessible in API.
  • There are some propagators which use the TraceState from core which could move to the API variant but this would require to bump the min API version there.
  • How to ensure compatibility between old/new implementation? I think it's likely to end up having both are in used within a single app.

@srikanthccv
Copy link
Member Author

@dyladan 's recommendation was to move this initially and then update wherever it is used. Encourage people to use the api method and issue depreciation notice to core component. But we will continue to keep maintaining the same changes in two places until the next major version.

@vmarchaud
Copy link
Member

vmarchaud commented Jan 5, 2022

Even though the core is GA we could just make a major bump, its supposed to be a internal package for instrumentations/sdks not something public facing right ?
In the order:

  • publish new api (as a minor ?)
  • update all usage from all packages to use the api instead of core
  • major bump core
  • update new version for all packages

EDIT: What i'm saying is that i believe we could do this, however i think its overkill for just moving this implementation

@Flarna
Copy link
Member

Flarna commented Jan 5, 2022

I think the first two steps are fine for now. Maybe add a doc deprecation of TraceState in core.

There are a few other issues open to move more from core to API. Maybe core gets anyway empty at some point in time...

@dyladan
Copy link
Member

dyladan commented Jan 5, 2022

Right now having all stable SDK packages have the same version makes it a lot easier for end users. Also, our main repo is not really well set up to have packages independently versioned. I think publishing a minor API, updating usage, and adding a deprecation notice in the core package is sufficient for now.

Before merging this, are there any unreleased PATCH level changes that we should release first? edit: looks like no

@vmarchaud vmarchaud mentioned this pull request Jan 11, 2022
src/trace/internal/tracestate-impl.ts Show resolved Hide resolved
src/trace/internal/validators.ts Outdated Show resolved Hide resolved
@legendecas legendecas merged commit 82842c7 into open-telemetry:main Jan 19, 2022
@dyladan dyladan added this to the v1.1 milestone Jan 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants