-
Notifications
You must be signed in to change notification settings - Fork 163
Opamp first iteration completion #2067
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?
Changes from 11 commits
7e4390f
a3d4a54
a710f38
cc57339
cf75d7e
f11a730
00d1ae1
e7d76b0
5e5fbeb
70f85cd
509e26f
4165d44
69b9977
d9a6cba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import ru.vyarus.gradle.plugin.animalsniffer.AnimalSniffer | ||
|
||
plugins { | ||
id("otel.java-conventions") | ||
id("ru.vyarus.animalsniffer") | ||
} | ||
|
||
dependencies { | ||
signature("com.toasttab.android:gummy-bears-api-21:0.12.0:coreLib@signature") | ||
} | ||
|
||
animalsniffer { | ||
sourceSets = listOf(java.sourceSets.main.get()) | ||
} | ||
|
||
// Always having declared output makes this task properly participate in tasks up-to-date checks | ||
tasks.withType<AnimalSniffer> { | ||
reports.text.required.set(true) | ||
} | ||
|
||
// Attaching animalsniffer check to the compilation process. | ||
tasks.named("classes").configure { | ||
finalizedBy("animalsnifferMain") | ||
} | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
|
||
package io.opentelemetry.opamp.client.internal.connectivity.http; | ||
|
||
import io.opentelemetry.api.internal.InstrumentationUtil; | ||
import java.io.IOException; | ||
import java.io.InputStream; | ||
import java.util.concurrent.CompletableFuture; | ||
|
@@ -13,6 +14,7 @@ | |
import okhttp3.Callback; | ||
import okhttp3.MediaType; | ||
import okhttp3.OkHttpClient; | ||
import okhttp3.Request; | ||
import okhttp3.RequestBody; | ||
import okio.BufferedSink; | ||
|
||
|
@@ -45,8 +47,14 @@ public CompletableFuture<Response> send(BodyWriter writer, int contentLength) { | |
RequestBody body = new RawRequestBody(writer, contentLength, MEDIA_TYPE); | ||
builder.post(body); | ||
|
||
InstrumentationUtil.suppressInstrumentation(() -> doSendRequest(builder.build(), future)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps there should be a comment explaining that this prevents automatic instrumentation from tracing the underlying okhttp request? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. I've just added a comment. |
||
|
||
return future; | ||
} | ||
|
||
private void doSendRequest(Request request, CompletableFuture<Response> future) { | ||
client | ||
.newCall(builder.build()) | ||
.newCall(request) | ||
.enqueue( | ||
new Callback() { | ||
@Override | ||
|
@@ -59,8 +67,6 @@ public void onFailure(Call call, IOException e) { | |
future.completeExceptionally(e); | ||
} | ||
}); | ||
|
||
return future; | ||
} | ||
|
||
private static class OkHttpResponse implements Response { | ||
|
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.
opentelemetry-java and opentelemetry-java-instrumentation don't have this
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.
Got it. It's just a way of getting notified of an api compatibility issue during development without having to run all the build checks, though it's not strictly necessary, so I've just removed it.
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 don't have anything particular against it just that it would be preferable to have this aligned with our other repositories.