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

Use OkHttp for AWS resource detectors #3991

Merged
merged 1 commit into from
Dec 15, 2021

Conversation

anuraaga
Copy link
Contributor

With OTLP exporters having a default dependency on OkHttp now, the vast majority of users will have OkHttp available on the classpath. There isn't a great reason to stick to HttpUrlConnection anymore - notably this will prevent resource detection requests from being traced during agent startup due to open-telemetry/opentelemetry-java-instrumentation#4642

assertThat(result).isEmpty();
}

@Test
void requestBody() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deleted the code path taking a request body as that was only used by the moved-out xray sampler code

@codecov
Copy link

codecov bot commented Dec 14, 2021

Codecov Report

Merging #3991 (ddebd09) into main (cbecf1f) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3991      +/-   ##
============================================
+ Coverage     89.71%   89.76%   +0.05%     
- Complexity     4272     4274       +2     
============================================
  Files           513      513              
  Lines         12941    12941              
  Branches       1248     1248              
============================================
+ Hits          11610    11617       +7     
+ Misses          927      924       -3     
+ Partials        404      400       -4     
Impacted Files Coverage Δ
...entelemetry/exporter/jaeger/PostSpansResponse.java 0.00% <0.00%> (-100.00%) ⬇️
...exporter/jaeger/MarshalerCollectorServiceGrpc.java 85.71% <0.00%> (-4.77%) ⬇️
...telemetry/sdk/trace/export/BatchSpanProcessor.java 88.48% <0.00%> (+0.71%) ⬆️
...ntelemetry/sdk/extension/resources/OsResource.java 90.69% <0.00%> (+4.65%) ⬆️
...ava/io/opentelemetry/sdk/internal/RateLimiter.java 100.00% <0.00%> (+5.88%) ⬆️
...metry/sdk/extension/resources/ProcessResource.java 87.50% <0.00%> (+6.25%) ⬆️
...elemetry/sdk/extension/resources/HostResource.java 92.30% <0.00%> (+15.38%) ⬆️
...dk/extension/resources/ProcessRuntimeResource.java 100.00% <0.00%> (+16.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbecf1f...ddebd09. Read the comment docs.

import okhttp3.ResponseBody;

/** A simple HTTP client based on OkHttp. Not meant for high throughput. */
final class SimpleHttpClient {
Copy link

Choose a reason for hiding this comment

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

Total NIT: as it's bound with OkHttp for good and bad I'd name it: "SimpleOkHttpClient" to note that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might end up swapping OkHttp back to HTTPUrlConnection or something else later, next time won't have to change class name references everywhere by using a more generic name here :)

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM :)

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.

2 participants