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.

Sign up for GitHub

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

Jump to bottom

Add missing InstrumenterBuilder.addRequestListener() #5655

Merged
merged 1 commit into from
Mar 22, 2022
Merged

Add missing InstrumenterBuilder.addRequestListener() #5655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions ...ion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,11 @@ public void end(

if (!requestListeners.isEmpty() || !requestMetricListeners.isEmpty()) {
long endNanos = getNanos(endTime);
for (RequestListener requestListener : requestListeners) {
// TODO (trask) call end in the reverse order that start was called?
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't have to do that - unlike start(), the end() method cannot modify the context; also, we're passing the timestamp as an argument, so the exact time of invocation should not matter too.

Copy link
Member Author

Choose a reason for hiding this comment

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

ya, I just always think of start/end pairs telescoping :)

for (RequestListener requestListener : requestMetricListeners) {
requestListener.end(context, attributes, endNanos);
}
for (RequestListener requestListener : requestMetricListeners) {
for (RequestListener requestListener : requestListeners) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: do we actually need two lists of listeners? Can't we merge requestMetricListeners into requestListeners?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll send that in a separate PR b/c it moves the requestListeners to after the span is started (which I think is ok, but I'd like to get eyes specifically on that change)

requestListener.end(context, attributes, endNanos);
}
}
Expand Down
8 changes: 7 additions & 1 deletion .../src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,13 @@ public InstrumenterBuilder<REQUEST, RESPONSE> addContextCustomizer(
return this;
}

/** Adds a {@link RequestMetrics} whose metrics will be recorded for request start and stop. */
/** Adds a {@link RequestListener} which will be called for request start and end. */
public InstrumenterBuilder<REQUEST, RESPONSE> addRequestListener(RequestListener listener) {
requestListeners.add(listener);
return this;
}

/** Adds a {@link RequestMetrics} whose metrics will be recorded for request start and end. */
@UnstableApi
public InstrumenterBuilder<REQUEST, RESPONSE> addRequestMetrics(RequestMetrics factory) {
requestMetricListeners.add(factory.create(meter));
Expand Down
32 changes: 32 additions & 0 deletions ...api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,38 @@ void client_parent() {
.hasParentSpanId("090a0b0c0d0e0f00")));
}

@Test
void requestListeners() {
AtomicReference<Boolean> startContext = new AtomicReference<>();
AtomicReference<Boolean> endContext = new AtomicReference<>();

RequestListener requestListener =
new RequestListener() {
@Override
public Context start(Context context, Attributes startAttributes, long startNanos) {
startContext.set(true);
return context;
}

@Override
public void end(Context context, Attributes endAttributes, long endNanos) {
endContext.set(true);
}
};

Instrumenter<Map<String, String>, Map<String, String>> instrumenter =
Instrumenter.<Map<String, String>, Map<String, String>>builder(
otelTesting.getOpenTelemetry(), "test", unused -> "span")
.addRequestListener(requestListener)
.newServerInstrumenter(new MapGetter());

Context context = instrumenter.start(Context.root(), REQUEST);
instrumenter.end(context, REQUEST, RESPONSE, null);

assertThat(startContext.get()).isTrue();
assertThat(endContext.get()).isTrue();
}

@Test
void requestMetrics() {
AtomicReference<Context> startContext = new AtomicReference<>();
Expand Down