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

Spans do not provide enough information #911

Closed
BrynCooke opened this issue Apr 25, 2022 · 3 comments
Closed

Spans do not provide enough information #911

BrynCooke opened this issue Apr 25, 2022 · 3 comments

Comments

@BrynCooke
Copy link
Contributor

Currently some of our spans are rather sparse. See the Otel spec for guidance.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#span

@BrynCooke BrynCooke changed the title Spans so not provide enough information Spans do not provide enough information Apr 25, 2022
@thrawny
Copy link

thrawny commented May 17, 2022

Some things I have noticed when testing the router and the opentelemetry stuff:

The span called fetch is of the wrong kind, right now it is internal but should be client, as described in the spec; "Indicates that the span describes a request to some remote service. This span is usually the parent of a remote SERVER span and does not end until the response is received."

I think the fetch span should be also include data about the remote service that is called;
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/span-general.md. For example net.peer.name.

The span called request is also the wrong kind, it is internal but should be server. In addition, I think it should include data about the incoming graphql request. Things that we have added in our gateway instrumentation are for example; operationName, variables and clientName.

@as-nikolaym
Copy link

I have some requests related to this, from the perspective of Jaeger.

In Jaeger, when you open a top-level trace of router, it looks a bit like this:

| router request
|__ router graphql_request
...
---|__ router fetch
------|__ graphql-serviceA MyOperationName_serviceA_1
---|__ router fetch
------|__ graphql-serviceB MyOperationName_serviceB_2

  • could the request label of the top-level span be made to be the operation name instead (MyOperationName in this example)? That will allow searching by the operation name in Jaeger.
  • the subgraph requests (to graphql-serviceA and graphql-serviceB in this example) get a running number (index) appended after their label (serviceA_1 and serviceB_2 in this case). Is that something that's needed or could it be removed? At least removing it will again help with Jaeger search - Jaeger's UI does not let me search by partial operation name. I assume this running number could change depending on what the particular GQL query contains, or in case a particular part of the schema gets moved from one subgraph to another. So if the running number changes, we won't necessarily see all relevant search results (at least not all at once).

@bnjjj
Copy link
Contributor

bnjjj commented Jun 15, 2022

Due to additions made in #1119 I think we can close this issue. Feel free to re-open it if needed.

@bnjjj bnjjj closed this as completed Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants