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: adding nested input variables in graphql plugin #720

Merged
merged 1 commit into from
Nov 8, 2021
Merged

fix: adding nested input variables in graphql plugin #720

merged 1 commit into from
Nov 8, 2021

Conversation

bodymindarts
Copy link
Contributor

@bodymindarts bodymindarts commented Oct 30, 2021

Which problem is this PR solving?

GraphQL inputs are frequently nested like:

{
  "input": {
    "value1": 1,
    "value2": 2
  }
}

Currently this would result the following call:

span.setAttribute("graphql.variables.input", Object({ value1: 1, value2: 2}))

But since SpanAttribute doesn't accept an Object type it silently fails
and doesn't add the attribute at all.

Short description of the changes

This PR goes depth-first into the nested structure and adds each
variable with the appropriate path like:

span.setAttribute("graphql.variables.input.value1", 1)
span.setAttribute("graphql.variables.input.value2", 2)

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

one concern to improve, other than that lgtm

@bodymindarts
Copy link
Contributor Author

bodymindarts commented Nov 5, 2021

Force pushed with suggested modification

EDIT:
and rebased against main

@dyladan
Copy link
Member

dyladan commented Nov 5, 2021

Please use conventional commits in your PR title as they are used to autogenerate new versions when packages are released. Your current title is not in the correct format and would be ignored, causing the package to not be released. If it is a bugfix that should only modify the PATCH version, please use feat: message_here. Otherwise, please use feat: message_here.

@codecov
Copy link

codecov bot commented Nov 5, 2021

Codecov Report

Merging #720 (bca78e8) into main (62a04c7) will not change coverage.
The diff coverage is n/a.

❗ Current head bca78e8 differs from pull request most recent head 8290025. Consider uploading reports for the commit 8290025 to get more accurate results

@@           Coverage Diff           @@
##             main     #720   +/-   ##
=======================================
  Coverage   96.87%   96.87%           
=======================================
  Files          11       11           
  Lines         640      640           
  Branches      126      126           
=======================================
  Hits          620      620           
  Misses         20       20           

@bodymindarts bodymindarts changed the title Fix adding nested input variables in graphql plugin fix: adding nested input variables in graphql plugin Nov 5, 2021
@bodymindarts
Copy link
Contributor Author

Force pushed to amend commit message and fix linting.

@dyladan
Copy link
Member

dyladan commented Nov 5, 2021

Thanks. Fixing the commit message doesn't matter as much as the PR title since the commits get squashed anyway and the PR title is used as the commit message.

GraphQL inputs are frequently nested like:
{
  "input": {
    "value1": 1,
    "value2": 2
  }
}

Currently this would result the following call:
span.setAttribute("input", Object({ value1: 1, value2: 2}))
But since SpanAttribute doesn't accept an Object type it silently fails
and doesn't add the attribute at all.

This commit goes depth-first into the nested structure and adds each
variable with the appropriate path.
@bodymindarts
Copy link
Contributor Author

Another attempt to fix linting

@vmarchaud vmarchaud merged commit 7a7d3a3 into open-telemetry:main Nov 8, 2021
@vmarchaud vmarchaud added the bug Something isn't working label Nov 8, 2021
@dyladan dyladan mentioned this pull request Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants