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

Un-escape url coding when parsing baggage. #2529

Merged
merged 3 commits into from
Jan 24, 2022
Merged

Un-escape url coding when parsing baggage. #2529

merged 3 commits into from
Jan 24, 2022

Conversation

MadVikingGod
Copy link
Contributor

Closes #2519

The W3c Sepc indicates that the value of a baggage member is a URL encoded string. We currently escape properly when we encode into the header, but we don't unescape when we praise from a header.

This change fixes that and adds tests at the propagator, and baggage level to ensure.

@codecov
Copy link

codecov bot commented Jan 21, 2022 •
edited
Loading

Codecov Report

Merging #2529 (5cd9107) into main (d1b6a7d) will decrease coverage by 0.1%.
The diff coverage is 50.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2529     +/-   ##
=======================================
- Coverage   76.0%   75.9%   -0.2%     
=======================================
  Files        174     174             
  Lines      12088   12093      +5     
=======================================
- Hits        9198    9189      -9     
- Misses      2647    2658     +11     
- Partials     243     246      +3
Impacted Files Coverage Δ
baggage/baggage.go 97.2% <50.0%> (-1.1%) ⬇️
sdk/metric/refcount_mapped.go 80.0% <0.0%> (-20.0%) ⬇️
sdk/trace/batch_span_processor.go 79.4% <0.0%> (-2.0%) ⬇️
sdk/metric/sdk.go 80.0% <0.0%> (-1.5%) ⬇️

Aneurysm9
Aneurysm9 approved these changes Jan 21, 2022
View reviewed changes
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

Please add a changelog entry, otherwise LGTM.

@MrAlias MrAlias merged commit 5f41868 into open-telemetry:main Jan 24, 2022
@MrAlias MrAlias mentioned this pull request Feb 10, 2022
Merged
MrAlias added a commit that referenced this pull request Mar 21, 2022
@vibhavp @jmacd
@ken8203 @bvwells @jeremy-stytch @MrAlias @thinkgos @MadVikingGod @dependabot @sincejune @chalin @dmathieu
Allow setting the Sampler via environment variables (#2517)
fdbcf71 
* Allow setting the Sampler via environment variables (#2305)

* Add changelog entry.

* Replace t.Setenv with internaltest/SetEnvVariables for Go <= 1.6.

* Handle the lack of a sampler argument without logging errors.

* Add additional test cases and error checks.

* Refactor documentation.

Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>

* emitBatchOverhead should only be used for splitting spans into batches (#2512)

* emitBatchOverhead should only be used for splitting spans into batches (#2503)

* limit max packet size parameter

* Add additional errors types, simplify abstractions and error handling

* Make error comparisons less fragile.

* Fix typo in jaeger example (#2524)

* Fix some typos in docs for Go libraries (#2520)

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

* Fix getting-started.md Run function (#2527)

* Fix getting-started.md Run function, it assigns this new context to a variable shared between connections in to accept loop. Thus creating a growing chain of contexts. so every calculate fibonacci request, all spans in a trace.

* add a comment explaining the reason for that new variable

* update example fib

* Bump github.com/google/go-cmp from 0.5.6 to 0.5.7 across the project (#2545)

* update go-cmp to 0.5.7

* fixes go.sums

Co-authored-by: Aaron Clawson <MadVikingGod@users.noreply.github.com>

* Un-escape url coding when parsing baggage. (#2529)

* un-escape url coding when parsing baggage.

* Added changelog

Co-authored-by: Aaron Clawson <MadVikingGod@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

* Bump go.opentelemetry.io/proto/otlp from 0.11.0 to 0.12.0 (#2546)

* Update go.opentelemetry.io/proto/otlp to v0.12.0

* Changelog

* Update CHANGELOG.md

Fix's md linting

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

Co-authored-by: Aaron Clawson <MadVikingGod@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

* Remove unused sdk/internal/santize (#2549)

* Add links to code examples and docs (#2551)

* Bump github.com/prometheus/client_golang from 1.11.0 to 1.12.0 in /exporters/prometheus (#2541)

* Bump github.com/prometheus/client_golang in /exporters/prometheus

Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.11.0 to 1.12.0.
- [Release notes](https://github.com/prometheus/client_golang/releases)
- [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md)
- [Commits](prometheus/client_golang@v1.11.0...v1.12.0)

---
updated-dependencies:
- dependency-name: github.com/prometheus/client_golang
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* go mod tidy

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tyler Yahn <codingalias@gmail.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

* Optimize evictedQueue implementation and use (#2556)

* Optimize evictedQueue impl and use

Avoid unnecessary allocations in the recordingSpan by using an
evictedQueue type instead of a pointer to one.

Lazy allocate the evictedQueue queue to prevent unnecessary operations
for spans without any use of the queue.

Document the evictedQueue

* Fix grammar

* Add env support for batch span processor (#2515)

* Add env support for batch span processor

* Update changelog

* lint

* Bump golang.org/x/tools from 0.1.8 to 0.1.9 in /internal/tools (#2566)

* Bump golang.org/x/tools from 0.1.8 to 0.1.9 in /internal/tools

Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.1.8 to 0.1.9.
- [Release notes](https://github.com/golang/tools/releases)
- [Commits](golang/tools@v0.1.8...v0.1.9)

---
updated-dependencies:
- dependency-name: golang.org/x/tools
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Auto-fix go.sum changes in dependent modules

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: MrAlias <MrAlias@users.noreply.github.com>

* Bump github.com/golangci/golangci-lint from 1.43.0 to 1.44.0 in /internal/tools (#2567)

* Bump github.com/golangci/golangci-lint in /internal/tools

Bumps [github.com/golangci/golangci-lint](https://github.com/golangci/golangci-lint) from 1.43.0 to 1.44.0.
- [Release notes](https://github.com/golangci/golangci-lint/releases)
- [Changelog](https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md)
- [Commits](golangci/golangci-lint@v1.43.0...v1.44.0)

---
updated-dependencies:
- dependency-name: github.com/golangci/golangci-lint
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Auto-fix go.sum changes in dependent modules

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: MrAlias <MrAlias@users.noreply.github.com>

* Bump github.com/prometheus/client_golang from 1.12.0 to 1.12.1 in /exporters/prometheus (#2570)

* Bump github.com/prometheus/client_golang in /exporters/prometheus

Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.12.0 to 1.12.1.
- [Release notes](https://github.com/prometheus/client_golang/releases)
- [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md)
- [Commits](prometheus/client_golang@v1.12.0...v1.12.1)

---
updated-dependencies:
- dependency-name: github.com/prometheus/client_golang
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Auto-fix go.sum changes in dependent modules

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: MrAlias <MrAlias@users.noreply.github.com>

* Fix TestBackoffRetry in otlp/internal/retry package (#2562)

* Fix TestBackoffRetry in otlp retry pkg

The delay of the retry is within two times a randomization factor (the
back-off time is delay * random number within [1 - factor, 1 + factor].
This means the waitFunc in TestBackoffRetry needs to check the delay is
within an appropriate delta, not equal to configure initial delay.

* Fix delta value

* Fix delta

Co-authored-by: Aaron Clawson <Aaron.Clawson@gmail.com>

* Bump google.golang.org/grpc from 1.43.0 to 1.44.0 in /exporters/otlp/otlptrace (#2568)

* Bump google.golang.org/grpc in /exporters/otlp/otlptrace

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.43.0 to 1.44.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.43.0...v1.44.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Auto-fix go.sum changes in dependent modules

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: MrAlias <MrAlias@users.noreply.github.com>

* Bump google.golang.org/grpc from 1.43.0 to 1.44.0 in /example/otel-collector (#2565)

* Bump google.golang.org/grpc in /example/otel-collector

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.43.0 to 1.44.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.43.0...v1.44.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Auto-fix go.sum changes in dependent modules

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: MrAlias <MrAlias@users.noreply.github.com>

* Bump google.golang.org/grpc from 1.43.0 to 1.44.0 in /exporters/otlp/otlpmetric (#2572)

* Bump google.golang.org/grpc in /exporters/otlp/otlpmetric

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.43.0 to 1.44.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.43.0...v1.44.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Auto-fix go.sum changes in dependent modules

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: MrAlias <MrAlias@users.noreply.github.com>

* Change Options to accept type not pointer (#2558)

* Change trace options to accept type not pointer

Add benchmark to show allocation improvement.

* Update CONTRIBUTING.md guidelines

* Update all Option iface

* Fix grammar in CONTRIBUTING

* Do not store TracerProvider or Tracer fields in SDK recordingSpan (#2575)

* Do not store TracerProvider fields in span

Instead of keeping a reference to the span's Tracer, and therefore also
it's TracerProvider, and the associated resource and spanLimits just
keep the reference to the Tracer. Refer to the TracerProvider fields
when needed instead.

* Make span refer to the inst lib via the Tracer

Instead of holding a field in the span, refer to the field in the parent
Tracer.

* [website_docs] fix page meta-links (#2580)

Contributes to open-telemetry/opentelemetry.io#1096

/cc @cartermp @austinlparker

* Validate members once, in `NewMember` (#2522)

* use NewMember, or specify if the member is not validated when creating new ones

* expect members to already be validated when creating a new package

* add changelog entry

* add an isEmpty field to member and property for quick validation

* rename isEmpty to hasData

So by default, an empty struct really is marked as having no data

* Update baggage/baggage_test.go

Co-authored-by: Aaron Clawson <Aaron.Clawson@gmail.com>

* don't validate the member in parseMember, we alredy ran that validation

We also don't want to use NewMember, as that runs the property
validation again, making the benchmark quite slower

* move changelog entry to the fixed section

* provide the member/property data when returning an invalid error

Co-authored-by: Aaron Clawson <Aaron.Clawson@gmail.com>

* Fix link to Zipkin exporter (#2581)

Currently it is linked to the old package that was moved.

* Unexport EnvBatchSpanProcessor* constants (#2583)

* Move BSP env support to internal

* Use pkg name

* Update env test

* Use internal/env in sdk/trace

* Avoid an extra allocation in applyTracerProviderEnvConfigs.

* Add additional errors for ratio > 1.0.

* Add test cases for ratio > 1.0.

* Update CHANGELOG.md

Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
Co-authored-by: jaychung <ken8203@gmail.com>
Co-authored-by: Ben Wells <b.v.wells@gmail.com>
Co-authored-by: Jeremy Kaplan <jeremy@stytch.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: thinkgo <49174849+thinkgos@users.noreply.github.com>
Co-authored-by: Aaron Clawson <Aaron.Clawson@gmail.com>
Co-authored-by: Aaron Clawson <MadVikingGod@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tyler Yahn <codingalias@gmail.com>
Co-authored-by: Chao Weng <19381524+sincejune@users.noreply.github.com>
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Co-authored-by: Damien Mathieu <42@dmathieu.com>
@dmathieu dmathieu mentioned this pull request Apr 4, 2022
Closed
@yurishkuro yurishkuro mentioned this pull request Feb 8, 2023
Open
yurishkuro
yurishkuro reviewed Feb 8, 2023
View reviewed changes
baggage/baggage_test.go
@@ -339,6 +339,13 @@ func TestBaggageParse(t *testing.T) {
"foo": {Value: "2"},
},
},
{
name: "url encoded value",
in: "key1=val%252",
Copy link
Member

Choose a reason for hiding this comment

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

This test was insufficient. The regexp check now happens after QueryUnescape, not at all what was intended in the original code. As a result, valid strings like val%27 do not pass the check. This specific test does pass because the decoded string val%2 still "looks like" URL-encoded (i.e. it passes the regex).

Copy link
Member

Choose a reason for hiding this comment

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

#3685 (comment)

@MadVikingGod MadVikingGod deleted the fix-baggage-urlencoding branch February 21, 2023 19:57
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro left review comments

@MrAlias MrAlias MrAlias approved these changes

@jmacd jmacd jmacd approved these changes

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

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

@evantorrie evantorrie Awaiting requested review from evantorrie

@paivagustavo paivagustavo Awaiting requested review from paivagustavo

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

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
untracked
Development

Successfully merging this pull request may close these issues.

Baggage member values are not being decoded when extracted from carriers
6 participants
@MadVikingGod @Aneurysm9 @yurishkuro @jmacd @MrAlias @pellared