Add path prefix for otlp http receiver

[fredthomsen]

Description:
Supports multiple otlp http receivers running behind a reverse proxy/ingress.

Fixes #7511

Testing:
Updated existing tests to account for new parameter.

Documentation:
Added documentation for configuring prefix in the receiver README.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.75 ⚠️

Comparison is base (7606f99) 90.96% compared to head (f063538) 90.21%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7570      +/-   ##
==========================================
- Coverage   90.96%   90.21%   -0.75%     
==========================================
  Files         300      301       +1     
  Lines       15090    15402     +312     
==========================================
+ Hits        13726    13895     +169     
- Misses       1091     1221     +130     
- Partials      273      286      +13     

Impacted Files	Coverage Δ
receiver/otlpreceiver/config.go	97.36% <100.00%> (+2.92%)	⬆️
receiver/otlpreceiver/factory.go	76.56% <100.00%> (+1.98%)	⬆️
receiver/otlpreceiver/otlp.go	82.94% <100.00%> (ø)

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[atoulme]

LGTM. The only thing is maybe this should be offered at the confighttp.HTTPServerSettings level? A maintainer should have a better view.

[fredthomsen]

LGTM. The only thing is maybe this should be offered at the confighttp.HTTPServerSettings level? A maintainer should have a better view.

Looking over this, yeah I agree. This would be better served having the PathPrefix as a field inside of confighttp.HTTPServerSettings. I'll make those updates.

Edit: @atoulme might have leaned a bit too far over my skis here. Is the above what you meant?

[jpkrohling]

I like this change, especially as it's forward-compatible with a possible generalization of the path prefix into the HTTPServerSettings. Given that no other receivers would use the path prefix, I'm against adding it directly there.

Let's add it only for OTLP Receiver, see the feedback, and generalize it if needed.

[fredthomsen]

@jpkrohling ok, I've made the updates you requested.

[bogdandrutu]

@jpkrohling @atoulme @fredthomsen please make sure we are consistent with the exporter option do configure this, see https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/otlphttpexporter/config.go#L26

I think we should do something like that instead, since the next request will be to change the "fixed" path. So I think in terms of something like having the full path traces_path, metrics_path, logs_path which will replace the entire path and default to otlp defaults. What do you think?

[fredthomsen]

@jpkrohling @atoulme @fredthomsen please make sure we are consistent with the exporter option do configure this, see https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/otlphttpexporter/config.go#L26

I think we should do something like that instead, since the next request will be to change the "fixed" path. So I think in terms of something like having the full path traces_path, metrics_path, logs_path which will replace the entire path and default to otlp defaults. What do you think?

So, I think there is two options here:

• Just having endpoints per signal type and dropping a command path prefix
• Keep common path prefix, and then allow configuration to override http paths per signal type.
  I lean towards the second option, given I think that setting individual http signal paths will be more infrequent .

[bogdandrutu]

Keep common path prefix, and then allow configuration to override http paths per signal type.
I lean towards the second option, given I think that setting individual http signal paths will be more infrequent .

Having two ways to configure something is usually confusing and harder to maintain. I would just vote for having per signal path, as proven in the exporter users did not complain about the ability (yet) to have a prefix, and we can always add that if we get such strong feedback that it is necessary to have that and that per signal is not working to them.

[jpkrohling]

I agree that the ideal is to have three endpoints for the HTTP receiver as well.

[fredthomsen]

I agree that the ideal is to have three endpoints for the HTTP receiver as well.

@jpkrohling @bogdandrutu I've moved to three separate url path configurations. I was waffling between validating the path config up front or not given that there is a panic caused by an invalid path.

[jpkrohling]

LGTM, I tested with a config like this:

receivers:
  otlp/1:
    protocols:
      grpc:

  otlp/2:
    protocols:
      http:
        traces_url_path: /traces

exporters:
  otlphttp/2:
    traces_endpoint: http://localhost:4318/traces
  logging:

service:
  pipelines:
    traces/1:
      receivers: [ otlp/1 ]
      processors:
      exporters: [ otlphttp/2 ]
    traces/2:
      receivers: [ otlp/2 ]
      processors:
      exporters: [ logging ]

I did have some trouble in finding the right config, so perhaps the following could be added to the readme:

1. explicitly mention in the readme how and where to set the endpoints. This is optional, as people can look into the test data for reference
2. state that, on the other side, the OTLP HTTP exporter should use the similar attributes for an end-to-end solution.

I wasn't able to get the SDK to work with this, including the telemetrygen. If you can make it work with that, it would be worth adding instructions here as well.

Those are things that can be done in a follow-up PR. This one here LGTM.

[fredthomsen]

@jpkrohling

1. explicitly mention in the readme how and where to set the endpoints. This is optional, as people can look into the test data for reference

There was information in the README present already, but it had the incorrect configuration parameter names. I have updated them now.

2. state that, on the other side, the OTLP HTTP exporter should use the similar attributes for an end-to-end solution.

Agreed. I have added this information.

I wasn't able to get the SDK to work with this, including the telemetrygen. If you can make it work with that, it would be worth adding instructions here as well.

Those are things that can be done in a follow-up PR. This one here LGTM.

I am not too familiar w/telemetrygen, so yeah let's tackle that in a follow up.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[fredthomsen]

Surprised to see these unit test failures as locally my runs are working fine.

[fredthomsen]

I stand corrected. I see it now.

Surprised to see these unit test failures as locally my runs are working fine.

[jpkrohling]

@open-telemetry/collector-maintainers, would one of you please review/merge this one?