-
Notifications
You must be signed in to change notification settings - Fork 278
fix: issue with sending traces to IPv6 endpoints #1935
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
base: main
Are you sure you want to change the base?
fix: issue with sending traces to IPv6 endpoints #1935
Conversation
|
|
||
| def http_connection(uri, ssl_verify_mode, certificate_file, client_certificate_file, client_key_file) | ||
| http = Net::HTTP.new(uri.host, uri.port) | ||
| http = Net::HTTP.new(uri.hostname, uri.port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening this PR, @chen-anders! I appreciate the Reviewer's note. The tests look good to me, but I think we should add them to the other libraries getting the IPv6 compatible hostname too. Could you update the tests in otlp-http, otlp-logs and otlp-metrics too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and added the tests requested for the other exporters.
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
| stub_request(:post, 'http://[::1]:4318/v1/traces').to_return(status: 200) | ||
| exp = OpenTelemetry::Exporter::OTLP::Exporter.new(endpoint: 'http://[::1]:4318/v1/traces') | ||
| span_data = OpenTelemetry::TestHelpers.create_span_data | ||
| result = exp.export([span_data]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @chen-anders thank you for pushing this along.
Overall looks good, but might need to consider the performance of the numerous test additions which use exp.export(...), which could lead to test pipeline performance slowdown.
Also I believe that the of actual exporting of span data is covered by other tests, so these new tests should only be concerned with parser validation such as:
http = exp.instance_variable_get(:@http)
_(http.address).must_equal '::1'
_(http.port).must_equal 4318
Do you agree @kaylareopelle ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and removed the tests in 0d18d61
* release: Release 5 gems * opentelemetry-sdk 1.10.0 (was 1.9.0) * opentelemetry-common 0.23.0 (was 0.22.0) * opentelemetry-exporter-otlp 0.31.0 (was 0.30.0) * opentelemetry-test-helpers 0.7.0 (was 0.6.0) * opentelemetry-metrics-sdk 0.10.0 (was 0.9.1) * Apply suggestions from code review --------- Co-authored-by: otelbot <[email protected]> Co-authored-by: Kayla Reopelle <[email protected]>
These were missed on earlier updates related to OTel security requirements
* release: Release 2 gems * opentelemetry-exporter-otlp-metrics 0.6.1 (was 0.6.0) * opentelemetry-exporter-otlp-logs 0.2.2 (was 0.2.1) * Apply suggestions from code review --------- Co-authored-by: otelbot <[email protected]> Co-authored-by: Kayla Reopelle <[email protected]>
…attribute (open-telemetry#1942) Fixes open-telemetry#1941 Co-authored-by: Kayla Reopelle <[email protected]>
) * test: test case for metric_stream and store * update test case * refine the test case; safeguard the callback * remove typo * replace unsafe timeout.timeout with thread join time * test fix * sleep 0.2 * skip some test for truffleruby * update test case * lint * skip thread related test * remove unstable test due to thread management * lint --------- Co-authored-by: Kayla Reopelle <[email protected]>
…emetry#1944) Co-authored-by: Kayla Reopelle <[email protected]>
* release: Release 3 gems * opentelemetry-exporter-otlp 0.31.1 (was 0.31.0) * opentelemetry-exporter-zipkin 0.24.1 (was 0.24.0) * opentelemetry-metrics-sdk 0.10.1 (was 0.10.0) * Update metrics_sdk/CHANGELOG.md --------- Co-authored-by: otelbot <[email protected]> Co-authored-by: Kayla Reopelle <[email protected]>
…1947) * Adds some debug log output for successfully exporting metrics, and additional error logging. Implementation is borrowed from the Batch Log Record Processor. Does not include a count. Eventually that can be handled with the `otel.sdk.exporter.metric_data_point.exported` internal metric. See https://opentelemetry.io/docs/specs/semconv/otel/sdk-metrics/ Fixes: open-telemetry#1888 * Add a test * remove rubocop disable * use endless syntax * remove unused RaisingExporter copied from BLRP test * don't set export interval or timeout * use let statements * Use public force_flush instead of internal export method * Remove skip for Windows platform * remove puts stmt from debugging
…telemetry#1950) Co-authored-by: otelbot <[email protected]>
0d18d61 to
ef12dac
Compare
|
Resolves: #1721
This PR resolves the issue around using the opentelemetry-ruby SDK for apps that send traces to an IPv6 endpoint using the suggested fix in the original issue. We also add some tests for verifying the expected value of the collector hostname based on a passed-in value.
Reviewer's Note: Tests were written with significant AI assistance.