Support http/json protocol for trace

[ramgdev]

Fixes # Design discussion issue (if applicable) #

Changes

Please provide a brief description of the changes here.

Merge requirement checklist

• [ ] CONTRIBUTING guidelines followed
• [ ] Unit tests added/updated (if applicable)
• [ ] Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• [ ] Changes in public API reviewed (if applicable)

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: ramgdev / name: Ramji (f9abf2401dfa039444b3179b995d1cd68e1f37bd, da76734f0b04b8464e43785f2da981bafa010ac2, f93058c197c223b7d00d6fb623a5eebcb7fe8e8b, 359babed332d92a58d9ce0aff2a527f38c880a96, e457a85f3618bc584a3ad004ab019214cae2cc79, ad8322baea40f0fcb64853c8cb044ca50a10dec7, 576a56d18afc8331942bc0e011e78711c88fca40, 25b0857dcfe0f5eb9f2908a40bb696457504a1ea, ed4fc0346ceb4088075786bc7d766bab76f0fb4a, e50ac0ee82f292d2aee312f3b59d4dd192b582d5, 049e96a5048a6b4ce348875a9e8a1c5b0dd3e628, 587c341f6b42109108cefdeb0d43cbbda7c69e9b, 5d5e06eb0bb198ab05449398193e0a976d59656f, 46f92c42ce766ff22a26b00c9d519a8a04614931, 04593ece4acb87569e903fc3df4e72d3af5ab6cb, 277df671e89b15926e038be60510aba3de9f1ff4, 5222f8d5b531ed012371260b9adc7c053ea21472, bcde320f730b3a0ef325b868187dbb5eaa21c664, 63fb044685521375a7a55517ad480c67d68febb1, 0f1a805c767bf43cad1b87d61b7cf033d66ef14a, efa8588bda77f5d41ba9849e8fc01ae619e59736, d64bda0f99a12e571c7c8b85649e28925aba2baa, e47a80f7519345bc61181df3e37ca6ec40947f5a, f17598064cfa9165f00e767ae5376987b5e81cdd, d13ab4b22b2ffe4caa887ec9f17bd8fc7d474e01, d460bda47217b73408b670842f1f4c6a5476543a, 1482d5ca6a9ec80aef265acd6c7dc66561456deb, 67d687591d98400d23b4f289f3f7ede88ef5ab7b, 6ebea63445076ffd4cd488902714eaeb3564f963, 8c9577f221562c26e75638fb19ccef593e69b4d5, ebc3cc3178c5e15d1e51c1737fc88f4b9b0970aa, 4ed5158dfad4096e84dc27aed38ed768f1c5a6c9, 67552f807800c8f85f47018d5f3a6cb490128d72, 765ab994a321fd80e42f4df6e4bb91ed1a16b9e4, 86a89d5a6aa126c327cdafd4776deb1be2b43a2d, c6d181750dbf18023ec278d4caac267b56e950a5, 250fab497aabe5ef3d13c2a7da7d2deeab1fc8c5, aa74b9cf180edbaf96b580c65986836965e49004)
• :white_check_mark: login: TommyCpp / name: Zhongyang Wu (aa2cd053a7a0354bbbcb313cae06e1f0813925c5)
• :white_check_mark: login: lalitb / name: Lalit Kumar Bhasin (680b1915ddf67beface29acbdd419181bb16fb92)

[codecov[bot]]

Codecov Report

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

Project coverage is 69.0%. Comparing base (f203b03) to head (250fab4). Report is 12 commits behind head on main.

:exclamation: Current head 250fab4 differs from pull request most recent head aa2cd05. Consider uploading reports for the commit aa2cd05 to get more accurate results

Files	Patch %	Lines
opentelemetry-otlp/src/exporter/http/logs.rs	0.0%	8 Missing :warning:
opentelemetry-otlp/src/exporter/http/metrics.rs	0.0%	8 Missing :warning:
opentelemetry-otlp/src/exporter/http/trace.rs	93.6%	3 Missing :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1585     +/-   ##
=======================================
- Coverage   69.3%   69.0%   -0.3%     
=======================================
  Files        136     136             
  Lines      19637   19517    -120     
=======================================
- Hits       13610   13483    -127     
- Misses      6027    6034      +7     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[lalitb]

@ramgdev Thanks for volunteering to add the support. This looks to be in the right direction, and good to review once ready.

[ramgdev]

@TommyCpp I agree users should be able to enable both http-proto and http-json features on the crate. I'll test it and perhaps use the protocol to decide on certain things in the code. Good catch.

[ramgdev]

@TommyCpp are there existing tests covering the function build_body in http exporters?

[TommyCpp]

build_body

Not exactly for the http exporter but in opentelemetry-proto we have this test https://github.com/open-telemetry/opentelemetry-rust/blob/e98c61ce73865b56917695dc0ab89ba3eca6c454/opentelemetry-proto/tests/json_deserialize.rs

[ramgdev]

The latest failures do not show up on pre-commit tests locally.

running 11 tests
test exporter::config::agent::tests::set_socket_address ... ok
test exporter::config::collector::http_client::collector_client_tests::test_bring_your_own_client ... ignored
test exporter::config::collector::tests::test_collector_exporter ... ignored
test exporter::config::collector::tests::test_resolve_endpoint ... ok
test exporter::config::collector::tests::test_resolve_timeout ... ok
test exporter::config::collector::tests::test_set_collector_endpoint ... ok
test exporter::config::tests::test_read_from_env ... ok
test exporter::config::tests::test_set_service_name ... ok
test exporter::tests::error_message_should_contain_details ... ok
test exporter::tests::ignores_user_set_values ... ok
test exporter::tests::test_set_status ... ok

test result: ok. 9 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 0.00s

[ramgdev]

Pulling back from review to fix test failures when running cargo test --all.

[ramgdev]

@TommyCpp please take a look when you get a chance.

[ramgdev]

Overall looks good. My concern is mostly around http-json overriding with http-proto when both are enabled

That's where I had most trouble changing the code. It seems that having a single default protocol is the source of the confusion. One cannot have both http-json and http-photo unless they attach it to a specific signal, right? Ultimately the export is going to happen on only one format. That means we have to build this default using a key that combines both format and signal rather than just the format.

[TommyCpp]

It seems that having a single default protocol is the source of the confusion.

Yeah I think it's reasonable to rely on features if the user didn't provide any configuration on which protocol to use. But we should allow(or even promote) configure the protocol explicitly

[ramgdev]

I think it's reasonable to rely on features

This is still a bit tricky when both http-json and http-proto features are enabled. Are we ok to pick http-json in that case? Or should we require the protocol to be specified when building the pipeline for http?

[TommyCpp]

Are we ok to pick http-json in that case? Or should we require the protocol to be specified when building the pipeline for http?

Yeah I think it's OK for us to pick http-json if user didn't provide one. i.e have a feature override in default_protocol. But in builder we should give users option to override it. Let me know if it makes sense

[ramgdev]

@TommyCpp sorry, just got to this. Please take a look when you get a chance. Thanks.

[cijothomas]

Thanks @ramgdev for your first PR! Hoping to see many more, if your time/passion permits!