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

Use url.PathUnescape when parsing OTLP headers and resource attributes env vars #4699

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

mikeblum
Copy link
Contributor

@mikeblum mikeblum commented Nov 8, 2023

…headers and resource attributes env vars (#4698).

A note: we might want to consolidate the table test data across grpc and html as there is a bit of drift / inconsistency in the test cases for each protocol?

EDIT: 😅 I modified the generated files initially:

#4699 (comment)

Copy link

linux-foundation-easycla bot commented Nov 8, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: mikeblum / name: Michael Blum (7011c66)

@mikeblum
Copy link
Contributor Author

mikeblum commented Nov 8, 2023

prior art: #4667

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #4699 (7011c66) into main (0c5ebd5) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4699     +/-   ##
=======================================
- Coverage   81.8%   81.8%   -0.1%     
=======================================
  Files        225     225             
  Lines      18087   18087             
=======================================
- Hits       14798   14796      -2     
- Misses      2992    2994      +2     
  Partials     297     297             
Files Coverage Δ
...ric/otlpmetricgrpc/internal/envconfig/envconfig.go 86.4% <100.0%> (ø)
...ric/otlpmetrichttp/internal/envconfig/envconfig.go 86.4% <100.0%> (ø)
...race/otlptracegrpc/internal/envconfig/envconfig.go 86.4% <100.0%> (ø)
...race/otlptracehttp/internal/envconfig/envconfig.go 86.4% <100.0%> (ø)
sdk/resource/env.go 94.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@mikeblum mikeblum force-pushed the GH-4698-url.PathUnescape branch 2 times, most recently from 5eba783 to 192db26 Compare November 8, 2023 22:01
@Aneurysm9
Copy link
Member

Please run make precommit to ensure formatting is correctly applied.

@pellared
Copy link
Member

pellared commented Nov 9, 2023

Can you please add some tests cases that would fail for url.QueryUnescape, but are now passing thanks to url.PathUnescape?

EDIT: For OTLP it would be adding test cases to TestStringToHeader, for resource it should be a new test in sdk/resource/env_test.go

@mikeblum
Copy link
Contributor Author

mikeblum commented Nov 9, 2023

Ah I see what happened; I had modified the generated table tests here vs the src in internal/shared/otlp/envconfig/envconfig_test.go.tmpl. Looking at the RFC 3986 vs net/url.PathUnescape it seems like the only diff are how + are handled?

Anyways this should cut down on the duplication i had originally.

@mikeblum mikeblum force-pushed the GH-4698-url.PathUnescape branch 2 times, most recently from 706ca80 to e7bc26d Compare November 9, 2023 11:56
sdk/resource/env_test.go Outdated Show resolved Hide resolved
sdk/resource/env_test.go Outdated Show resolved Hide resolved
@pellared pellared changed the title Use url.PathUnescape rather than url.QueryUnescape when parsing OTLP … Use url.PathUnescape when parsing OTLP headers and resource attributes env vars Nov 9, 2023
@mikeblum
Copy link
Contributor Author

mikeblum commented Nov 9, 2023

Checked CONTRIBUTING.md but I wasn't sure if squashing was enabled or not for PRs so I did it manually. Thanks for pushing up your suggestions!

@MrAlias MrAlias merged commit be5064a into open-telemetry:main Nov 9, 2023
25 checks passed
@pellared
Copy link
Member

@mikeblum Thank you for your contribution 👍

@XSAM XSAM added this to the Old Untracked PRs milestone Nov 7, 2024
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.

6 participants