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

fix(gql)!: conform GraphQL span name to spec #1444

Merged
merged 5 commits into from
Apr 2, 2023

Conversation

naseemkullah
Copy link
Member

@naseemkullah naseemkullah commented Mar 29, 2023

Which problem is this PR solving?

Rename graphql "execute" spans to "<operation.type> <operation.name>" to conform to the spec.

In addition to conforming to the spec, this change can provide additional value for monitoring purposes. In cases where o11y providers generate metrics from pre-ingested traces (without all the attributes), the new naming convention makes it easier to group and filter metrics by operation type and name, since it is now reflected in the span name. This can be particularly useful for setting up monitors or alerts.

Short description of the changes

When operation name and type are determined within the execute span, span.updateName() is called.

The query steps other than "execute"'s spans are out of scope as they
do not need to be renamed.

@github-actions github-actions bot requested a review from obecny March 29, 2023 01:28
@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #1444 (5622c65) into main (861b867) will decrease coverage by 1.31%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1444      +/-   ##
==========================================
- Coverage   96.13%   94.82%   -1.31%     
==========================================
  Files          14        2      -12     
  Lines         906       58     -848     
  Branches      197        4     -193     
==========================================
- Hits          871       55     -816     
+ Misses         35        3      -32     

see 16 files with indirect coverage changes

@naseemkullah naseemkullah marked this pull request as ready for review March 29, 2023 01:42
@naseemkullah naseemkullah requested a review from a team March 29, 2023 01:42
@naseemkullah naseemkullah force-pushed the graphql-span-name branch 7 times, most recently from 392765d to 87d3fd2 Compare March 29, 2023 14:50
Rename graphql "execute" spans to "<operation.type> <operation.name>"
to conform to the spec.

In addition to conforming to the spec, this change can provide
additional value for monitoring purposes. In cases where o11y providers
generate metrics from pre-ingested traces (without all the attributes),
the new naming convention makes it easier to group and filter metrics
by operation type and name, since it is now reflected in the span name.
This can be particularly useful for setting up monitors or alerts.

The query steps other than "execute"'s spans are out of scope as they
do not need to be renamed.
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Overall LGTM

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM, but I have one question about the changelog. I see there's a changelog for each package, and for contrib as a whole. Are they generated automatically? Is there something we can do to make it completely clear to users that the span name will change after upgrading?

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

👍 Changelogs are created automatically on release PRs.

Other than emphasizing it in the changelog and bumping major(which we cannot do, because 0.x), I don't know how to make it more clear, especially since we're fixing spec noncompliance.

@pichlermarc
Copy link
Member

+1 Changelogs are created automatically on release PRs.

Other than emphasizing it in the changelog and bumping major(which we cannot do, because 0.x), I don't know how to make it more clear, especially since we're fixing spec noncompliance.

I also think there's nothing we can do to make it clearer. I think it should be categorized correctly as a breaking change in the changelog when titling the PR as fix(gql)!: conform GraphQL span name to spec, so maybe changing the title could help.

@naseemkullah naseemkullah changed the title fix(gql): conform GraphQL span name to spec fix(gql)!: conform GraphQL span name to spec Mar 31, 2023
@naseemkullah
Copy link
Member Author

naseemkullah commented Mar 31, 2023

+1 Changelogs are created automatically on release PRs.
Other than emphasizing it in the changelog and bumping major(which we cannot do, because 0.x), I don't know how to make it more clear, especially since we're fixing spec noncompliance.

I also think there's nothing we can do to make it clearer. I think it should be categorized correctly as a breaking change in the changelog when titling the PR as fix(gql)!: conform GraphQL span name to spec, so maybe changing the title could help.

Thanks, I was unsure whether this was considered breaking or not since, as @rauno56 mentioned, we are adhering to spec. In any case ! added as you suggested @pichlermarc.
I'll likely merge by Monday morning EST if there are no further concerns.

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks good 🙂

@naseemkullah naseemkullah enabled auto-merge (squash) April 2, 2023 18:41
@naseemkullah naseemkullah merged commit 7d070db into open-telemetry:main Apr 2, 2023
@dyladan dyladan mentioned this pull request Apr 2, 2023
@naseemkullah naseemkullah deleted the graphql-span-name branch April 3, 2023 12:21
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.

7 participants