Stop percent-encoding the header environment variables in otlplog exporters #6392

tongoss · 2025-03-02T06:39:28Z

Bugfixes for #5623

Based on the conversation #5623 (comment), only OTLP log exporters need bugfixes.

linux-foundation-easycla · 2025-03-02T06:39:31Z

The committers listed above are authorized under a signed CLA.

✅ login: tongoss / name: Robert Wu (337a320, bcf5fec, 84dccd6, 9cbacc6, 28b03f4, 0a50e3e, 52d8272, 0d956e5, c6f0fa3, a4e2d2e, bfbb5aa, edcfc43, 2d57f4c, fe68066, fb22d99, 6889ded)
✅ login: dmathieu / name: Damien Mathieu (532fb77)
✅ login: pellared / name: Robert Pająk (3816fc5)

dmathieu

This needs to be tested.
I'd recommend looking at https://github.com/open-telemetry/opentelemetry-go/pull/5705/files for inspiration on how to write those tests.

CHANGELOG.md

codecov · 2025-03-03T22:55:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (fb89a38) to head (bfbb5aa).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #6392     +/-   ##
=======================================
- Coverage   81.8%   81.8%   -0.1%     
=======================================
  Files        283     283             
  Lines      24899   24927     +28     
=======================================
+ Hits       20379   20400     +21     
- Misses      4117    4122      +5     
- Partials     403     405      +2

see 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tongoss · 2025-03-03T23:00:15Z

This needs to be tested. I'd recommend looking at https://github.com/open-telemetry/opentelemetry-go/pull/5705/files for inspiration on how to write those tests.

@dmathieu , the required test cases have been added.

pellared · 2025-03-04T05:45:33Z

@tongoss, why have your closed the PR?

tongoss · 2025-03-04T12:14:15Z

@tongoss, why have your closed the PR?

@pellared , Thanks so much for reminding me. I happened to click the wrong button...

dmathieu

When errors are returned (and therefore handled), we shouldn't be logging them.

exporters/otlp/otlplog/otlploggrpc/config.go

exporters/otlp/otlplog/otlploghttp/config.go

exporters/otlp/otlplog/otlploggrpc/config.go

2. Fix the code in order to parse valid header keys only 3. Add global error messages

Co-authored-by: Damien Mathieu <[email protected]>

Apply the suggested changes Co-authored-by: Damien Mathieu <[email protected]>

exporters/otlp/otlplog/otlploggrpc/config.go

Co-authored-by: Robert Pająk <[email protected]>

…both invalid and valid headers exist

pellared

Good overall 👍 Just two comments related to unit tests.

exporters/otlp/otlplog/otlploghttp/config_test.go

…ders test case

tongoss · 2025-03-07T22:45:02Z

Good overall 👍 Just two comments related to unit tests.

@pellared , Thanks very much for the comments. They are all very helpful. I have pushed the new changes.

pellared · 2025-03-11T08:22:07Z

@open-telemetry/go-maintainers, planning to merge tomorrow.

# Overview Closes #6786 ### Added - Add exponential histogram support in `go.opentelemetry.io/otel/exporters/prometheus`. (#6421) - The `go.opentelemetry.io/otel/semconv/v1.31.0` package. The package contains semantic conventions from the `v1.31.0` version of the OpenTelemetry Semantic Conventions. See the [migration documentation](./semconv/v1.31.0/MIGRATION.md) for information on how to upgrade from `go.opentelemetry.io/otel/semconv/v1.30.0`. (#6479) - Add `Recording`, `Scope`, and `Record` types in `go.opentelemetry.io/otel/log/logtest`. (#6507) - Add `WithHTTPClient` option to configure the `http.Client` used by `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`. (#6751) - Add `WithHTTPClient` option to configure the `http.Client` used by `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp`. (#6752) - Add `WithHTTPClient` option to configure the `http.Client` used by `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#6688) - Add `ValuesGetter` in `go.opentelemetry.io/otel/propagation`, a `TextMapCarrier` that supports retrieving multiple values for a single key. (#5973) - Add `Values` method to `HeaderCarrier` to implement the new `ValuesGetter` interface in `go.opentelemetry.io/otel/propagation`. (#5973) - Update `Baggage` in `go.opentelemetry.io/otel/propagation` to retrieve multiple values for a key when the carrier implements `ValuesGetter`. (#5973) - Add `AssertEqual` function in `go.opentelemetry.io/otel/log/logtest`. (#6662) - The `go.opentelemetry.io/otel/semconv/v1.32.0` package. The package contains semantic conventions from the `v1.32.0` version of the OpenTelemetry Semantic Conventions. See the [migration documentation](./semconv/v1.32.0/MIGRATION.md) for information on how to upgrade from `go.opentelemetry.io/otel/semconv/v1.31.0`(#6782) - Add `Transform` option in `go.opentelemetry.io/otel/log/logtest`. (#6794) - Add `Desc` option in `go.opentelemetry.io/otel/log/logtest`. (#6796) ### Removed - Drop support for [Go 1.22]. (#6381, #6418) - Remove `Resource` field from `EnabledParameters` in `go.opentelemetry.io/otel/sdk/log`. (#6494) - Remove `RecordFactory` type from `go.opentelemetry.io/otel/log/logtest`. (#6492) - Remove `ScopeRecords`, `EmittedRecord`, and `RecordFactory` types from `go.opentelemetry.io/otel/log/logtest`. (#6507) - Remove `AssertRecordEqual` function in `go.opentelemetry.io/otel/log/logtest`, use `AssertEqual` instead. (#6662) ### Changed - ⚠️ Update `github.com/prometheus/client_golang` to `v1.21.1`, which changes the `NameValidationScheme` to `UTF8Validation`. This allows metrics names to keep original delimiters (e.g. `.`), rather than replacing with underscores. This can be reverted by setting `github.com/prometheus/common/model.NameValidationScheme` to `LegacyValidation` in `github.com/prometheus/common/model`. (#6433) - Initialize map with `len(keys)` in `NewAllowKeysFilter` and `NewDenyKeysFilter` to avoid unnecessary allocations in `go.opentelemetry.io/otel/attribute`. (#6455) - `go.opentelemetry.io/otel/log/logtest` is now a separate Go module. (#6465) - `go.opentelemetry.io/otel/sdk/log/logtest` is now a separate Go module. (#6466) - `Recorder` in `go.opentelemetry.io/otel/log/logtest` no longer separately stores records emitted by loggers with the same instrumentation scope. (#6507) - Improve performance of `BatchProcessor` in `go.opentelemetry.io/otel/sdk/log` by not exporting when exporter cannot accept more. (#6569, #6641) ### Deprecated - Deprecate support for `model.LegacyValidation` for `go.opentelemetry.io/otel/exporters/prometheus`. (#6449) ### Fixes - Stop percent encoding header environment variables in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc` and `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#6392) - Ensure the `noopSpan.tracerProvider` method is not inlined in `go.opentelemetry.io/otel/trace` so the `go.opentelemetry.io/auto` instrumentation can instrument non-recording spans. (#6456) - Use a `sync.Pool` instead of allocating `metricdata.ResourceMetrics` in `go.opentelemetry.io/otel/exporters/prometheus`. (#6472) --------- Co-authored-by: Tyler Yahn <[email protected]> Co-authored-by: Robert Pająk <[email protected]>

tongoss requested review from MrAlias, XSAM, dashpole, pellared and dmathieu as code owners March 2, 2025 06:39

tongoss force-pushed the bugfix/otlp-exporter-logs-env branch from e1bfe9a to dd3776a Compare March 2, 2025 07:08

dmathieu changed the title ~~Bugfix/otlp exporter logs env~~ Stop percent-encoding the header environment variables in otlplog exporters Mar 3, 2025

dmathieu requested changes Mar 3, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

tongoss requested a review from dmathieu March 4, 2025 00:26

tongoss closed this Mar 4, 2025

tongoss reopened this Mar 4, 2025

dmathieu reviewed Mar 4, 2025

View reviewed changes

tongoss requested a review from dmathieu March 4, 2025 17:53

dmathieu approved these changes Mar 5, 2025

View reviewed changes

pellared reviewed Mar 5, 2025

View reviewed changes

exporters/otlp/otlplog/otlploggrpc/config.go Outdated Show resolved Hide resolved

tongoss force-pushed the bugfix/otlp-exporter-logs-env branch from 3d55867 to 2904171 Compare March 6, 2025 03:57

tongoss and others added 9 commits March 6, 2025 08:06

bug fix

52d8272

1. Add more unit tests

337a320

2. Fix the code in order to parse valid header keys only 3. Add global error messages

update global.Error message

0a50e3e

Update exporters/otlp/otlplog/otlploggrpc/config.go

0d956e5

Co-authored-by: Damien Mathieu <[email protected]>

Update exporters/otlp/otlplog/otlploggrpc/config.go

bcf5fec

Apply the suggested changes Co-authored-by: Damien Mathieu <[email protected]>

Update exporters/otlp/otlplog/otlploggrpc/config.go

28b03f4

Apply the suggested changes Co-authored-by: Damien Mathieu <[email protected]>

Update exporters/otlp/otlplog/otlploghttp/config.go

fb22d99

Apply the suggested changes Co-authored-by: Damien Mathieu <[email protected]>

Update exporters/otlp/otlplog/otlploghttp/config.go

9cbacc6

Apply the suggested changes Co-authored-by: Damien Mathieu <[email protected]>

Update exporters/otlp/otlplog/otlploghttp/config.go

c6f0fa3

Apply the suggested changes Co-authored-by: Damien Mathieu <[email protected]>

Add global warning message before nullify the error

edcfc43

tongoss force-pushed the bugfix/otlp-exporter-logs-env branch from 2904171 to edcfc43 Compare March 6, 2025 13:06

pellared reviewed Mar 6, 2025

View reviewed changes

exporters/otlp/otlplog/otlploggrpc/config.go Outdated Show resolved Hide resolved

tongoss and others added 3 commits March 6, 2025 17:57

Update exporters/otlp/otlplog/otlploggrpc/config.go

6889ded

Co-authored-by: Robert Pająk <[email protected]>

Remove the code for nullifying err. Update unit test: return none if …

fe68066

…both invalid and valid headers exist

Merge branch 'main' into bugfix/otlp-exporter-logs-env

84dccd6

pellared reviewed Mar 7, 2025

View reviewed changes

exporters/otlp/otlplog/otlploghttp/config_test.go Outdated Show resolved Hide resolved

exporters/otlp/otlplog/otlploghttp/config_test.go Outdated Show resolved Hide resolved

Include TestConvHeaders for err assertion. Update percent-encoded hea…

2d57f4c

…ders test case

Merge branch 'main' into bugfix/otlp-exporter-logs-env

a4e2d2e

pellared requested a review from dmathieu March 10, 2025 22:20

pellared approved these changes Mar 10, 2025

View reviewed changes

dmathieu approved these changes Mar 11, 2025

View reviewed changes

Merge branch 'main' into bugfix/otlp-exporter-logs-env

3816fc5

dmathieu and others added 2 commits March 11, 2025 09:25

Merge branch 'main' into bugfix/otlp-exporter-logs-env

532fb77

Merge branch 'main' into bugfix/otlp-exporter-logs-env

bfbb5aa

XSAM approved these changes Mar 12, 2025

View reviewed changes

pellared merged commit efe325a into open-telemetry:main Mar 12, 2025
31 of 32 checks passed

MrAlias added this to the v1.36.0 milestone Mar 15, 2025

MrAlias mentioned this pull request Mar 26, 2025

OTLP exporters should not percent decode the key when parsing HEADERS env vars #5623

Closed

dmathieu mentioned this pull request May 19, 2025

Release v1.36.0/v0.58.0/v0.12.0 #6793

Merged

Stop percent-encoding the header environment variables in otlplog exporters #6392

Stop percent-encoding the header environment variables in otlplog exporters #6392

Uh oh!

Conversation

tongoss commented Mar 2, 2025

Uh oh!

linux-foundation-easycla bot commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmathieu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tongoss commented Mar 3, 2025

Uh oh!

pellared commented Mar 4, 2025

Uh oh!

tongoss commented Mar 4, 2025

Uh oh!

dmathieu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pellared left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tongoss commented Mar 7, 2025

Uh oh!

pellared commented Mar 11, 2025

Uh oh!

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Mar 2, 2025 •

edited

Loading

codecov bot commented Mar 3, 2025 •

edited

Loading