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

[Bug]: constructing a tonic builder defaults to connect to 4318? #1509

Closed
colemickens opened this issue Feb 3, 2024 · 6 comments
Closed
Labels
bug Something isn't working

Comments

@colemickens
Copy link

What happened?

I thought I saw the issue in the code last night, but now I'm not convinced I quite grok the issue, but here's what I'm seeing:

As part of my telemetry pipeline creation, I do this:

                let mut tonic = opentelemetry_otlp::new_exporter().tonic();
                let exporter = SpanExporterBuilder::Tonic(tonic);

However, this tries to connect to 4318 by default, rather than 4317 as I'd expect.

// Endpoints per protocol https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md
const OTEL_EXPORTER_OTLP_GRPC_ENDPOINT_DEFAULT: &str = "http://localhost:4317";
const OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT: &str = "http://localhost:4318";

Right now I have to do this:

                let mut tonic = opentelemetry_otlp::new_exporter().tonic();
                tonic.export_config().endpoint = "http://localhost:4317".to_string();
                let exporter = SpanExporterBuilder::Tonic(tonic);

but it doesn't feel right that I need to?

API Version

opentelemetry_sdk = { version = "0.21.2", features = ["rt-tokio"] }
opentelemetry-otlp = { version = "0.14", features = [
  "http-proto",
  "reqwest-client",
  "trace" # TODO: ensure we need this
] }
opentelemetry = "0.21"
tracing-opentelemetry = "0.22"

SDK Version

(see above)

What Exporters are you seeing the problem on?

OTLP

Relevant log output

DEBUG connection error: not connected
DEBUG resolving host="localhost"
DEBUG connecting to [::1]:4318
DEBUG connected to [::1]:4318
DEBUG binding client connection
DEBUG client connection bound
DEBUG send frame=Settings { flags: (0x0), enable_push: 0, initial_window_size: 2097152, max_frame_size: 16384 }
DEBUG service.ready=true processing request
DEBUG Connection: Connection::poll; connection error error=GoAway(b"", FRAME_SIZE_ERROR, Library) peer=Client
DEBUG Connection: send frame=GoAway { error_code: FRAME_SIZE_ERROR, last_stream_id: StreamId(0) } peer=Client
DEBUG Connection: Connection::poll; connection error error=GoAway(b"", FRAME_SIZE_ERROR, Library) peer=Client
DEBUG client request body error: error writing a body to connection: user error: inactive stream
DEBUG connection error: hyper::Error(Http2, Error { kind: GoAway(b"", FRAME_SIZE_ERROR, Library) })
DEBUG client response error: connection error detected: frame with invalid size
OpenTelemetry trace error occurred. Exporter otlp encountered the following error(s): the grpc server returns error (Unknown error): , detailed error message: h2 protocol error: http2 error: connection error detected: frame with invalid size
DEBUG connection error: not connected
DEBUG resolving host="localhost"
DEBUG connecting to [::1]:4318
DEBUG connected to [::1]:4318
DEBUG binding client connection
DEBUG client connection bound
DEBUG send frame=Settings { flags: (0x0), enable_push: 0, initial_window_size: 2097152, max_frame_size: 16384 }
DEBUG service.ready=true processing request
DEBUG Connection: Connection::poll; connection error error=GoAway(b"", FRAME_SIZE_ERROR, Library) peer=Client
DEBUG Connection: send frame=GoAway { error_code: FRAME_SIZE_ERROR, last_stream_id: StreamId(0) } peer=Client
DEBUG Connection: Connection::poll; connection error error=GoAway(b"", FRAME_SIZE_ERROR, Library) peer=Client
DEBUG client request body error: error writing a body to connection: send stream capacity unexpectedly closed
DEBUG connection error: hyper::Error(Http2, Error { kind: GoAway(b"", FRAME_SIZE_ERROR, Library) })
DEBUG client response error: connection error detected: frame with invalid size
DEBUG connection error: not connected
OpenTelemetry trace error occurred. Exporter otlp encountered the following error(s): the grpc server returns error (Unknown error): , detailed error message: h2 protocol error: http2 error: connection error detected: frame with invalid size
@colemickens colemickens added bug Something isn't working triage:todo Needs to be traiged. labels Feb 3, 2024
@colemickens
Copy link
Author

Right, it's this code here: https://docs.rs/opentelemetry-otlp/0.14.0/src/opentelemetry_otlp/exporter/mod.rs.html#119

  1. Having feature flags change defaults like this seems... bad/scary api design.
  2. I'd argue it's logically wrong. The selection of the default port is separate from the decision/selection about the protocol to be used, even though it should almost surely be dependent on that, rather than the crate features.

I'm not great at Rust API design (yet? fingers crossed, heh), but it seems like ExportConfig should maybe not have a Default impl and should require a new(Protocol) so the default can be set more appropriate

@cijothomas
Copy link
Member

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md As per the spec, gRPC should use 4317, and http/protobuf should use 4318.. So when changing the transport, it is expected that it'll change the port as well. Not sure if this has anything to do with OTel Rust in particular - this should be same across all languages implementing OTel spec.

@cijothomas cijothomas removed bug Something isn't working triage:todo Needs to be traiged. labels Feb 8, 2024
@colemickens
Copy link
Author

colemickens commented Feb 8, 2024

But I'm using GRPC* and it's defaulting to 4318...

@colemickens
Copy link
Author

colemickens commented Feb 8, 2024

Like, the code defines the correct default port for GRPC, and then proceeds to ignore it because I happen to have the http feature enabled in addition to grpc. It strikes me as wrong. (Or i'm gravely misunderstanding a critical point!)

@cijothomas
Copy link
Member

But I'm using GRPC* and it's defaulting to 4318...

oh.. that I believe could be a bug!

@cijothomas cijothomas added the bug Something isn't working label Feb 8, 2024
hdost added a commit to hdost/opentelemetry-rust that referenced this issue Feb 18, 2024
Current behavior uses http-proto port even when there's no desire to use
it.

Relates open-telemetry#1509
hdost added a commit to hdost/opentelemetry-rust that referenced this issue Feb 18, 2024
Current behavior uses http-proto port even when there's no desire to use
it.

Relates open-telemetry#1509
hdost added a commit to hdost/opentelemetry-rust that referenced this issue Feb 18, 2024
Current behavior uses http-proto port even when there's no desire to use
it.

Relates open-telemetry#1509

Signed-off-by: Harold Dost <h.dost@criteo.com>
hdost added a commit to hdost/opentelemetry-rust that referenced this issue Feb 18, 2024
Current behavior uses http-proto port even when there's no desire to use
it.

Relates open-telemetry#1509

Signed-off-by: Harold Dost <h.dost@criteo.com>
hdost added a commit to hdost/opentelemetry-rust that referenced this issue Feb 18, 2024
Current behavior uses http-proto port even when there's no desire to use
it.

Relates open-telemetry#1509

Signed-off-by: Harold Dost <h.dost@criteo.com>
hdost added a commit to hdost/opentelemetry-rust that referenced this issue Feb 18, 2024
Current behavior uses http-proto port even when there's no desire to use
it.

Relates open-telemetry#1509

Signed-off-by: Harold Dost <h.dost@criteo.com>
@TommyCpp
Copy link
Contributor

TommyCpp commented Feb 19, 2024

Fixed by #1556

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants