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

Keep router span name consistent and do not use operation name #5261

Closed
smyrick opened this issue May 28, 2024 · 6 comments · Fixed by #5365
Closed

Keep router span name consistent and do not use operation name #5261

smyrick opened this issue May 28, 2024 · 6 comments · Fixed by #5365
Assignees

Comments

@smyrick
Copy link
Member

smyrick commented May 28, 2024

Is your feature request related to a problem? Please describe.

The span name for the service entry point for Apollo Router OpenTelemtry tracing is <service_name>:<operation_name>. This causes many issues with my dashboard linking in DataDog as I now have a high cardinality of spans.

Screenshot 2024-05-28 at 2 57 58 PM

Screenshot 2024-05-28 at 2 59 04 PM

Describe the solution you'd like

It would be nice if instead we used just a standard name like <serice_name>:router or if this was configurable so I could set the span name myself. I would also like the operation name to be a configurable span attribute as I have to go to the supergraph to see this today. This is before the GraphQL parsing so what ever logic is being used to get the operation span name today should just become the attribute value

Describe alternatives you've considered

I could maybe use a custom collector and transform all the spans

@BrynCooke
Copy link
Contributor

BrynCooke commented Jun 3, 2024

We have followed the otel spec here: https://opentelemetry.io/docs/specs/semconv/graphql/graphql-spans/

The span name MUST be of the format <graphql.operation.type> <graphql.operation.name> provided that graphql.operation.type and graphql.operation.name are available. If graphql.operation.name is not available, the span SHOULD be named <graphql.operation.type>. When <graphql.operation.type> is not available, GraphQL Operation MAY be used as span name.

I think this would be a feature request to disable such behaviour.

@Samjin
Copy link

Samjin commented Jun 3, 2024

For each resource, APM automatically generates a dashboard page covering:

  • Key health metrics
  • Monitor status for all monitors associated with this service
  • List of metrics for all resources associated with this service

https://docs.datadoghq.com/tracing/glossary/#resources
https://docs.datadoghq.com/tracing/services/resource_page/

Datadog requires to use static span name with resource and it will automatically creates APM service page and other resource pages with key metrics. Although basic trace would still be present without static name or resource, many provided features wouldn't be available.

@BrynCooke
Copy link
Contributor

BrynCooke commented Jun 6, 2024

OK, sounds like we should add the config.
For reference here is someone asking for the opposite behaviour #3229.

I do think the behaviour mandated in the spec for changing the span names is really strange, and it's also noticeable that other parts of the spec don't seem to follow this pattern.

Suggest we support the following config:

telemetry:
  instrumentation:
    spans:
      router:
        otel.name: router # 

otel.name is the special attribute that can be used to override the span name. I am sort of surprised this doesn't work already, bunt it could be an ordering issue.

We could shift the logic out of the telemetry plugin into a selector maybe?

@bnjjj WDYT?

@BrynCooke BrynCooke removed their assignment Jun 6, 2024
@bnjjj
Copy link
Contributor

bnjjj commented Jun 6, 2024

I agree with your solution @BrynCooke, the main reason it doesn't work today is because it's kind of an ordering rule when at the end it takes the name directly set in the span_builder here, I already have to make that kind of fix for otel.status_code by introducing this and adding a special case here and here

@abernix
Copy link
Member

abernix commented Jun 7, 2024

@bnjjj and @BrynCooke Are there implications or user-facing-observed-changes if we were to just change that ordering around?

@bnjjj
Copy link
Contributor

bnjjj commented Jun 10, 2024

no

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants