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.

Sign up for GitHub

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

Jump to bottom

Fix validation for tracestate with vendor and add tests #1581

Merged
merged 3 commits into from
Feb 25, 2021
Merged

Fix validation for tracestate with vendor and add tests #1581

merged 3 commits into from
Feb 25, 2021

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Feb 24, 2021

TraceContext specification allows digits to be placed at the beginning of tracestate key for multi-tenant vendor scenario, this updates our implementation to match.

Additionally it adds test cases from the W3C unit tests to test this change and other regresions identified there.

Resolves #1475

@MrAlias MrAlias added pkg:API Related to an API package area:trace Part of OpenTelemetry tracing release:required-for-ga labels Feb 24, 2021
XSAM
XSAM approved these changes Feb 25, 2021
View reviewed changes
Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

👍

@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #1581 (18cedf5) into main (43886e5) will increase coverage by 0.0%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1581   +/-   ##
=====================================
  Coverage   77.6%   77.6%           
=====================================
  Files        128     128           
  Lines       6635    6635           
=====================================
+ Hits        5149    5155    +6     
+ Misses      1239    1233    -6     
  Partials     247     247
Impacted Files Coverage Δ
trace/trace.go 86.6% <ø> (ø)
sdk/trace/batch_span_processor.go 78.8% <0.0%> (+5.7%) ⬆️

@MrAlias MrAlias merged commit e499ca8 into open-telemetry:main Feb 25, 2021
@MrAlias MrAlias deleted the tracestate-multi-tenant-re branch February 25, 2021 15:36
@Aneurysm9 Aneurysm9 mentioned this pull request Mar 3, 2021
Merged
ldelossa pushed a commit to ldelossa/opentelemetry-go that referenced this pull request Mar 5, 2021
@MrAlias @ldelossa
Fix validation for tracestate with vendor and add tests (open-telemet…
5b4b6ec 
…ry#1581)

* Fix validation for tracestate with vendor

* Add changes to changelog
This was referenced Mar 7, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Mar 11, 2021
Open
Open
Open
Open
Open
Open
Open
Open
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Open
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@XSAM XSAM XSAM approved these changes

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

@evantorrie evantorrie Awaiting requested review from evantorrie

@jmacd jmacd Awaiting requested review from jmacd

@lizthegrey lizthegrey Awaiting requested review from lizthegrey

Assignees
No one assigned
Labels
area:trace Part of OpenTelemetry tracing pkg:API Related to an API package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update regular expression for traceStateKeyFormatWithMultiTenantVendor
3 participants
@MrAlias @Aneurysm9 @XSAM