[exporter/otlphttp] added support for configurable telemetry encoding

[tvaintrob]

Description:
This PR adds support for encoding configuration in the otlphttp exporter.

Link to tracking Issue:
#6945

Testing:
Updated existing tests, and added relevant tests

Documentation:
Updated the otlphttp docs to include the new configuration option.

[codecov]

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (ca0eab2) 90.45% compared to head (f724cb7) 90.34%.

Files	Patch %	Lines
exporter/otlphttpexporter/otlp.go	69.73%	23 Missing ⚠️
exporter/otlphttpexporter/config.go	76.92%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9276      +/-   ##
==========================================
- Coverage   90.45%   90.34%   -0.11%     
==========================================
  Files         346      346              
  Lines       18131    18194      +63     
==========================================
+ Hits        16400    16437      +37     
- Misses       1399     1424      +25     
- Partials      332      333       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[songy23]

Thanks @tvaintrob please also add a changelog

[bogdandrutu]

What is the use-case for this? I remember that we intentionally did not want to offer this, because it is just a slower version of the proto and you don't have the dependency issue like you have on the client side (web) to avoid depending on protobuf.

[tvaintrob]

@bogdandrutu in my case we have a WAF that we want to send messages through but it needs to do a deep validation of the messages being passed, unfortunately it doesn't support reading binary data so we need to send the messages using JSON

[tvaintrob]

@songy23 fixed all comments except for the additional tests

[songy23]

Please fix the failed tests https://github.com/open-telemetry/opentelemetry-collector/actions/runs/7513270300/job/20574065081?pr=9276

[songy23]

We can mention in the documentation that json should only be used in endpoints that can only accept json, but discouraged in other use cases, e.g. collector-to-collector case. Does that sound good? @bogdandrutu

[TylerHelmuth]

I agree with supporting http/json. Although it isn't performant, it feels correct to implement all parts of the OTLP specification for the exporter.

[songy23]

LGTM

cc @open-telemetry/collector-approvers PTAL

[mx-psi]

What is the use-case for this? I remember that we intentionally did not want to offer this, because it is just a slower version of the proto and you don't have the dependency issue like you have on the client side (web) to avoid depending on protobuf.

@bogdandrutu There was general agreement on last week's Collector SIG to add support for this. Do you think we should add something in the README about the performance implications?

[tvaintrob]

@TylerHelmuth updated the PR with the requested changes, not all are possible due to the issues with contrib

Thanks

[TylerHelmuth]

Thanks for opening open-telemetry/opentelemetry-collector-contrib#30703. I'd say we fix that before merging this PR.

[songy23]

Thanks for opening open-telemetry/opentelemetry-collector-contrib#30703. I'd say we fix that before merging this PR.

Turns out to be straightforward to fix it, I have open-telemetry/opentelemetry-collector-contrib#30759

Dismiss the approval now that blocker is removed factory.CreateDefaultConfig() needs updating

[tvaintrob]

Thanks for the update @songy23 ill update this one to include the default encoding

[songy23]

Minor comments, LGTM otherwise

[TylerHelmuth]

@tvaintrob please add some test cases that hit the new error scenarios and a test for the new UnmarshalText function.

[tvaintrob]

@TylerHelmuth @songy23 updated the PR, i rather take care of the extra error cases in a different PR, feels out of scope for this one

I'll open a ticket for it once this one is merged

[songy23]

LGTM, please file an issue to return error on invalid content-type in requests.

[tvaintrob]

LGTM, please file an issue to return error on invalid content-type in requests.

#9413 created an issue for that, thanks

[tvaintrob]

@TylerHelmuth just waiting on your approval to merge this one

Thanks!

[TylerHelmuth]

@tvaintrob could you add a could more test scenarios to cover the error cases Codecov is complaining about?

[tvaintrob]

@tvaintrob could you add a could more test scenarios to cover the error cases Codecov is complaining about?

I had a talk with @songy23 about it, he said that it's ok to leave it like that, do you have any example on how to generate malformed data?

[TylerHelmuth]

I think you'd have to call the private functions directly. I am ok moving forward without these tests since they cover such unlikely, simple scenarios

[mx-psi]

@bogdandrutu I will merge this on or after Wednesday next week EU morning. Please block by then if you think we should not merge this.

[sarathsp06]

LGTM

[mx-psi]

@tvaintrob please fix the merge conflict and I can merge afterwards :)

[tvaintrob]

@mx-psi Hi, resolved the merge conflict, thanks!