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

Support "http" protocol #7

Merged
merged 7 commits into from
Feb 2, 2024
Merged

Support "http" protocol #7

merged 7 commits into from
Feb 2, 2024

Conversation

lbeschastny
Copy link
Collaborator

No description provided.

src/otlp.rs Outdated Show resolved Hide resolved
src/otlp.rs Outdated Show resolved Hide resolved
#[cfg(feature = "integration_test")]
#[cfg(feature = "test")]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if I should rename this, but it's only used by profiles right now, so it should be fine

Comment on lines +37 to +38
hyper = { version = "0.14", default-features = false, features = ["http1", "client"], optional = true }
hyper-v1 = { package = "hyper", version = "1.1", default-features = false, features = ["http1", "client"], optional = true }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

opentelemetry-http uses hyper@0.14 and it's not really compatible with the latest one.

And we need hyper@1 for out test setup, but this one is hidden behind test feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps we should add an http feature and hide opentelemetry-http and hyper behind it.

But it will complicate protocol detection quite a lot since we'll have to insert a lot of cfg macros to handle cases when http support is disabled.

Copy link

@Travispersson Travispersson Feb 1, 2024

Choose a reason for hiding this comment

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

It might take some time for hyper 1.x support and progress can be followed here open-telemetry/opentelemetry-rust#1427

Copy link
Collaborator Author

@lbeschastny lbeschastny Feb 1, 2024

Choose a reason for hiding this comment

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

Yeah, looks like it.

I even tried to implement my own HyperClient for the latest hyper version, but it didn't work since new hyper uses http@1, but opentelemetry-http is written on top of hyper@0.14 and http@0.

@lbeschastny lbeschastny merged commit 8005dbe into main Feb 2, 2024
2 checks passed
@lbeschastny lbeschastny deleted the feature/allow-http-protocol branch February 2, 2024 09:39
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

3 participants