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

Otlp http exporter: allow endpoint override #2492

Conversation

rypdal
Copy link
Contributor

@rypdal rypdal commented Oct 18, 2021

Fixes #.

Changes

Implemented the following behaviour:

default path is only appended when the Options.Endpoint property wasn't set by the user, the protocol is HttpProtobuf and the OTEL_EXPORTER_OTLP_ENDPOINT environment variable is present. If the user provides a custom value for Options.Endpoint that value is taken as is.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

Sorry, something went wrong.

… metrics and adjusted them according to spec. when reading it from environment variables + allow Endpoint override
@rypdal rypdal requested a review from a team October 18, 2021 13:50
pellared
pellared previously approved these changes Oct 18, 2021
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.

I see some build errors but apart from it looks OK.

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

Just some nits on the docs. I guess there will be a separate work to add "support" for the per-signal env vars?

rypdal and others added 2 commits October 19, 2021 11:44

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
- improved descriptions and README for new added properties

Co-authored-by: Joao Grassi <joao@joaograssi.com>
@utpilla
Copy link
Contributor

utpilla commented Oct 19, 2021

@rypdal I have a few questions regarding spec compliance here . The spec mentions two scenarios for constructing URLs:

  1. For the per-signal variables (OTEL_EXPORTER_OTLP__ENDPOINT), the URL MUST be used as-is without any modification. The only exception is that if an URL contains no path part, the root path / MUST be used (see Example 2).

It looks like currently we are not reading from OTEL_EXPORTER_OTLP_TRACES_ENDPOINT or OTEL_EXPORTER_OTLP_METRICS_ENDPOINT. Is this planned for a future PR? Also, these variables should allow one to override the URL for sending a specific signal as shown in Example 3 of the spec. With the changes in this PR, the OtlpExporterOptions.TracesEndpoint and OtlpExporterOptions.MetricsEndpoint cannot have any value different from OtlpExporterOptions.Endpoint. Is this not expected to be covered by this PR?

  1. If signals are sent that have no per-signal configuration from the previous point, OTEL_EXPORTER_OTLP_ENDPOINT is used as a base URL and the signals are sent to these paths relative to that:
  • Traces: v1/traces
  • Metrics: v1/metrics.

Non-normatively, this could be implemented by ensuring that the base URL ends with a slash and then appending the relative URLs as strings.

When someone sets the OtlpExporterOptions.Endpoint property, do we not want to append "v1/traces" or "v1/metrics" to OtlpExporterOptions.TracesEndpoint and OtlpExporterOptions.MetricsEndpoint if the Protocol is HttpProtobuf?

@codecov
Copy link

codecov bot commented Oct 20, 2021

Codecov Report

Merging #2492 (c522b34) into main (116a101) will decrease coverage by 0.04%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2492      +/-   ##
==========================================
- Coverage   79.90%   79.86%   -0.05%     
==========================================
  Files         251      251              
  Lines        8939     8948       +9     
==========================================
+ Hits         7143     7146       +3     
- Misses       1796     1802       +6     
Impacted Files Coverage Δ
...nTelemetryProtocol/OtlpMetricExporterExtensions.cs 0.00% <0.00%> (ø)
...entation/ExportClient/OtlpHttpTraceExportClient.cs 90.47% <100.00%> (ø)
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs 96.00% <100.00%> (+0.44%) ⬆️
...metryProtocol/OtlpTraceExporterHelperExtensions.cs 73.68% <100.00%> (+3.09%) ⬆️
...ZPages/Implementation/ZPagesExporterEventSource.cs 56.25% <0.00%> (-6.25%) ⬇️
src/OpenTelemetry/BatchExportProcessor.cs 85.86% <0.00%> (-3.27%) ⬇️

- got rid of traces and metrics endpoints in exporter options as it should be considered along with per-signal env. vars. and out of scope of this issue
- traces and metrics relative paths append logic is located in corresponding AddOtlpExporter methods where call scope is known
@rypdal
Copy link
Contributor Author

rypdal commented Oct 20, 2021

@rypdal I have a few questions regarding spec compliance here . The spec mentions two scenarios for constructing URLs:

  1. For the per-signal variables (OTEL_EXPORTER_OTLP__ENDPOINT), the URL MUST be used as-is without any modification. The only exception is that if an URL contains no path part, the root path / MUST be used (see Example 2).

It looks like currently we are not reading from OTEL_EXPORTER_OTLP_TRACES_ENDPOINT or OTEL_EXPORTER_OTLP_METRICS_ENDPOINT. Is this planned for a future PR? Also, these variables should allow one to override the URL for sending a specific signal as shown in Example 3 of the spec. With the changes in this PR, the OtlpExporterOptions.TracesEndpoint and OtlpExporterOptions.MetricsEndpoint cannot have any value different from OtlpExporterOptions.Endpoint. Is this not expected to be covered by this PR?

  1. If signals are sent that have no per-signal configuration from the previous point, OTEL_EXPORTER_OTLP_ENDPOINT is used as a base URL and the signals are sent to these paths relative to that:
  • Traces: v1/traces
  • Metrics: v1/metrics.

Non-normatively, this could be implemented by ensuring that the base URL ends with a slash and then appending the relative URLs as strings.

When someone sets the OtlpExporterOptions.Endpoint property, do we not want to append "v1/traces" or "v1/metrics" to OtlpExporterOptions.TracesEndpoint and OtlpExporterOptions.MetricsEndpoint if the Protocol is HttpProtobuf?

@utpilla , please, see my answers/last changes below:

  1. I've decided to simplify code and removed traces and metrics endpoint properties from exporter options as it should be considered along with per-signal env. vars. Moreover, other per-signal env. vars. like OTEL_EXPORTER_OTLP_{Signal}HEADERS, OTEL_EXPORTER_OTLP{Signal}_PROTOCOL, etc should be considered in dedicated PR as well. The scope of this PR is only allow endpoint override. AFAIK PR for support of signal specific env. vars. not exists yet.
  2. Current implementation is changed in the way that traces and metrics relative paths append logic is moved to corresponding AddOtlpExporter methods where call scope (traces or metrics) is known.
  3. Specification is clear about appending of "v1/traces" or "v1/metrics" only for case when endpoint is read from OTEL_EXPORTER_OTLP_ENDPOINT. If user overwrites Endpoint then it stays as is without any changes. I think the main subject of this issue is to allow user to overwrite with it's own export endpoint.
  4. @cijothomas: with the current (simplified) approach the API Compatibility build doesn't fail anymore.

@pellared pellared dismissed their stale review October 21, 2021 09:06

I see a lot changes and I have no time to review it today. Sorry :(

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…rride
rypdal and others added 2 commits October 27, 2021 10:05

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…rride
rypdal and others added 4 commits November 5, 2021 12:17

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…rride

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…rride
Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @rypdal!

@alanwest alanwest merged commit 8b3501a into open-telemetry:main Nov 16, 2021
@rypdal
Copy link
Contributor Author

rypdal commented Nov 17, 2021

@cijothomas , @pellared , @alanwest , @utpilla , @joaopgrassi. @mic-max , @reyang thank you all for the review and discussions.

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.

None yet

8 participants