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

Support query params for traces url #5152

Closed

Conversation

nozik
Copy link

@nozik nozik commented Apr 4, 2024

Our OpenTelemetry collector is deployed behind an API gateway that we don't control. IThe API gateway requires passing a certain query parameter - which can't be moved to a header (and certainly not to the OTLP request body). For that reason, we need the http exporter to support adding query parameters.

@pellared
Copy link
Member

pellared commented Apr 4, 2024

Can you please add a description? Why do you want such functionality? Are other SDKs supporting this? It is not common to have query parameters in POST HTTP requests.

Take notice that the OpenTelemetry Specification does not require supporting query parameters. See: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md.

Some time ago it was even saying that it MUST NOT support it: open-telemetry/opentelemetry-specification#3739

@pellared pellared added the question Further information is requested label Apr 4, 2024
@nozik
Copy link
Author

nozik commented Apr 4, 2024

@pellared Added a description. From my experience query parameters are quite common in POST APIs (perhaps not a best practice), and I see no harm in adding them.

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 83.6%. Comparing base (5449f08) to head (9f44031).
Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5152     +/-   ##
=======================================
- Coverage   83.8%   83.6%   -0.2%     
=======================================
  Files        248     252      +4     
  Lines      16345   16443     +98     
=======================================
+ Hits       13709   13759     +50     
- Misses      2347    2394     +47     
- Partials     289     290      +1     
Files Coverage Δ
...trace/otlptracehttp/internal/otlpconfig/options.go 92.4% <100.0%> (+0.2%) ⬆️
exporters/otlp/otlptrace/otlptracegrpc/options.go 73.5% <0.0%> (-2.9%) ⬇️
exporters/otlp/otlptrace/otlptracehttp/client.go 78.7% <66.6%> (-0.6%) ⬇️
...trace/otlptracegrpc/internal/otlpconfig/options.go 90.4% <0.0%> (-3.3%) ⬇️

... and 6 files with indirect coverage changes

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Our OpenTelemetry collector is deployed behind an API gateway that we don't control. The API gateway requires passing a certain query parameter - which can't be moved to a header.

I do not support such change unless it is clearly specified in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md that the implementation SHOULD honor the query URL component.

Personally, I am against such proposal. Therefore, please try to address it yourself in the https://github.com/open-telemetry/opentelemetry-specification. I guess it would be still better (and maybe even easier) to change the API gateway.

For now, I am closing the PR. We can reopen it later if needed.

@pellared pellared added blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made and removed question Further information is requested labels Apr 4, 2024
@pellared pellared closed this Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants