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

Migrate GWT to Instrumenter API #3146

Merged
merged 5 commits into from
Jun 1, 2021

Conversation

anuraaga
Copy link
Contributor

This started as me wanting to remove a usage of SpanNames, but realized this can just be an RPC instrumentation.

import io.opentelemetry.instrumentation.api.instrumenter.rpc.RpcSpanNameExtractor;
import java.lang.reflect.Method;

public final class GwtSingletons {
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 think I've used every pattern possible for the name of this sort of class. I wonder if we should just converge on *Singletons so it can fit instrumenters, but also any other singletons that come our way.

Copy link
Member

Choose a reason for hiding this comment

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

works for me

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

It looks like the RPC semantic conventions specifically target CLIENT/SERVER spans

though I can definitely see a case for RPC conventions applied to RPC systems designed on top of servlets

or maybe that's what a "handler span" is? 😂

import io.opentelemetry.instrumentation.api.instrumenter.rpc.RpcSpanNameExtractor;
import java.lang.reflect.Method;

public final class GwtSingletons {
Copy link
Member

Choose a reason for hiding this comment

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

works for me

@anuraaga
Copy link
Contributor Author

I guess there's a difference between REST and RPC - I think we're only supposed to have one span here since this seems to be similar to gRPC. So I guess we should be updating any HTTP span with the RPC span name and attributes. A mode the instrumenter API doesn't yet support, and it ties into the general nested span stuff as usual :)

scope.close();

tracer().endSpan(context, throwable);
INSTRUMENTER.end(context, method, null, throwable);
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to use an instrumenter() static method here?

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

can you add a TODO somewhere about the GWT RPC span currently being an INTERNAL span?

@anuraaga anuraaga closed this Jun 1, 2021
@anuraaga anuraaga reopened this Jun 1, 2021
@anuraaga anuraaga merged commit 5cbade4 into open-telemetry:main Jun 1, 2021
robododge pushed a commit to robododge/opentelemetry-java-instrumentation that referenced this pull request Jun 17, 2021
* Migrate GWT to Instrumenter API

* TODO

* instrumenter()
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.

3 participants