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

[opentelemetry-otlp] adds an example HTTP exporter backed by Hyper #1861

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

markdingram
Copy link
Contributor

@markdingram markdingram commented Jun 4, 2024

Resolves #1659

Given the Pending Hyper 1.0 upgrade seems to make sense for Hyper 0.14 Client support to be provided as an example.

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)

@markdingram markdingram requested a review from a team June 4, 2024 07:59
Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.0%. Comparing base (028d3e7) to head (a4f018a).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1861   +/-   ##
=====================================
  Coverage   75.0%   75.0%           
=====================================
  Files        122     122           
  Lines      20279   20279           
=====================================
+ Hits       15211   15212    +1     
+ Misses      5068    5067    -1     

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

@cijothomas
Copy link
Member

From 6/4/ community call:
Please see if we can use the existing examples, and add feature flags to it to showcase diff. http clients, instead of maintaining full examples for each of them.

@cijothomas
Copy link
Member

From 6/4/ community call: Please see if we can use the existing examples, and add feature flags to it to showcase diff. http clients, instead of maintaining full examples for each of them.

@markdingram Would you be willing to change this based on this recommendation? Thanks in advance!

@markdingram
Copy link
Contributor Author

From 6/4/ community call: Please see if we can use the existing examples, and add feature flags to it to showcase diff. http clients, instead of maintaining full examples for each of them.

@markdingram Would you be willing to change this based on this recommendation? Thanks in advance!

I had a go

@@ -52,6 +52,14 @@ Run the app which exports logs, metrics and traces via OTLP to the collector
cargo run
```


By default the app will use a `reqwest` client to send. A hyper 0.14 client can be used with the `hyper` feature enabled
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
By default the app will use a `reqwest` client to send. A hyper 0.14 client can be used with the `hyper` feature enabled
By default the app will use a `reqwest` client to send. A hyper client can be used with the `hyper` feature enabled

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Thanks!
Will wait for one more approval to merge. (My own experience with hyper is limited, so need an extra pair of eyes)

@@ -0,0 +1,45 @@
use async_trait::async_trait;
Copy link
Member

@lalitb lalitb Jun 19, 2024

Choose a reason for hiding this comment

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

Do you think we can instead update the HttpConfig in opentelemetry-otlp to support the hyper client, and so use the hyper related code from opentelemetry-http. I don't know if hyper 1.0 is the blocker for that?

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

@markdingram please see if this makes sense - #1861 (comment)

@lalitb lalitb mentioned this pull request Jun 21, 2024
4 tasks
@markdingram
Copy link
Contributor Author

markdingram commented Jun 24, 2024

@markdingram please see if this makes sense - #1861 (comment)

Anything done now will conflict with #1674 - I will look again once the crate upgrades are through.

@lalitb
Copy link
Member

lalitb commented Jul 1, 2024

@markdingram please see if this makes sense - #1861 (comment)

Anything done now will conflict with #1674 - I will look again once the crate upgrades are through.

should be good, assuming we will revisit this once #1674 is through.

@cijothomas cijothomas merged commit d5c86d9 into open-telemetry:main Jul 1, 2024
25 checks passed
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.

[Bug]: unclear how to setup http(hyper) without agent pipeline
3 participants