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

gRPC instrumentation: client additions #269

Merged
merged 13 commits into from
Feb 5, 2021

Conversation

alertedsnake
Copy link
Contributor

@alertedsnake alertedsnake commented Dec 18, 2020

Description

Updates the gRPC client instrumentation:

  • Add additional attributes to span described in the spec but not previously added
  • fix example in docs that didn't work gRPC client instrumentation example doesn't run #256
  • rework the instrumentation call so both secure and insecure connections are instrumented by default - otherwise there's no way to handle both
  • properly capture and record RPC errors
  • change all metrics labels recorded to be strings
  • change metrics labels to be namespaced like the span attributes are, this seems to be a convention when looking at other client instrumentation (i.e. requests)

Re: the instrumentation call - I preserved the existing parameter so as to not break changes, but we may wish to mark that as deprecated, I don't know the convention with this library on how to do that.

Fixes #256

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

I've added new checks in the existing tests to make sure the attributes

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@alertedsnake alertedsnake marked this pull request as ready for review December 18, 2020 23:15
@alertedsnake alertedsnake requested review from a team, codeboten and ocelotl and removed request for a team December 18, 2020 23:15
@alertedsnake alertedsnake changed the title Grpc client additions gRPC instrumentation: client additions Dec 23, 2020
Base automatically changed from master to main January 29, 2021 19:28
@alertedsnake
Copy link
Contributor Author

Is there anything you need me to do to help the process along here? This gRPC part of this is not really usable as-is, and I'd like to help with other things, but I'm blocked by these PRs.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Code changes LGTM

Michael Stella and others added 10 commits February 4, 2021 13:03
Add tests for the new attributes
Bugfix for docs that led me astray for a good half hour :)
The docs on metric labels suggests that they should probably be strings,
and all others I can find are strings, and so these ought to be also.
Otherwise, some of the exporters/processors have to handle things
specifically, and not all of these come out as nice as could be when you
`str()` them.

I've also made sure to use the `StatusCode` name, as that's the
interesting thing.

Finally, there's no need to report specifically that `error=false`, so
I've removed that tag.
Hey, good thing there are tests.

Also, I did the `sorted()` thing in here so future us don't have to
think about the ordering of these when writing new tests :)
This code passed before, so something clearly changed.
@alertedsnake
Copy link
Contributor Author

The docker error in the tests seems entirely unrelated to the changes made in this PR, does anyone know how to fix?

@ocelotl
Copy link
Contributor

ocelotl commented Feb 4, 2021

The docker error in the tests seems entirely unrelated to the changes made in this PR, does anyone know how to fix?

I am investigating it 🔎

@alertedsnake
Copy link
Contributor Author

I am investigating it

Thanks!

# handle legacy argument
if "channel_type" in kwargs:
if kwargs.get("channel_type") == "secure":
return ("secure_channel",)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want to return different types (tuple or list)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I didn't even realize. I'll fix it, good catch.

@codeboten codeboten merged commit ade29f6 into open-telemetry:main Feb 5, 2021
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.

gRPC client instrumentation example doesn't run
3 participants