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

Add more tests, fix compliance to semantics #236

Merged
merged 10 commits into from
Dec 10, 2020

Conversation

alertedsnake
Copy link
Contributor

@alertedsnake alertedsnake commented Dec 5, 2020

Description

A whole bunch of reworking the tests

I've updated tests to use proper handler names - In the real world, the interceptor is always going to be passed a HandlerCallDetails object with the method field set to something valid... previously these tests set this to an empty string, but that's not what happens in a real case. So I've updated the tests to do that right, but also made sure that the span processing allows this to be unset, just in case someone was expecting this behavior to be present.

I've added more checks into the existing tests, now we're checking that span attributes are set as expected.

Fixed a few bugs in the tests where things which were intended to be checked, actually were not.

And finally, I've added a test to catch the gRPC abort() and verify the error status in the span.

Updated status codes to be compliant with specifications

The previous PR I made had the gRPC status codes using the English names for the status, but the spec has been clarified to require the integer value.

Also, the specs say to not use span.set_status() for a non-error condition, so I've added a check for that in a few places.

Added additional attributes

Also as the spec has been clarified, I've added the rpc.method and rpc.service attributes.

Fixes #173
Fixes #139

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)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

As most of this is either test changes or changes covered by new or existing tests, I've just run a bunch of tests :)

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 requested review from a team, toumorokoshi and hectorhdzg and removed request for a team December 5, 2020 17:26
@alertedsnake
Copy link
Contributor Author

(And now replying properly with my real github account)

Confirmed a slight issue - I'm setting the span.status.description field to the error description, but it seems that's expected to be type: detail in the case of an exception, which is a bit harder to do with gRPC, since any abort is raised as a bare Exception even when it isn't technically. I suppose I could make this end up something like StatusCode.NAME: description (with name and description set appropriately).

Anyone know where the canonical doc on that format is?

This shows up because of this line here: https://github.com/open-telemetry/opentelemetry-python-contrib/blob/master/exporter/opentelemetry-exporter-datadog/src/opentelemetry/exporter/datadog/exporter.py#L252

I did notice #154 and was pondering tackling that as well, so if this is considered an error, I can fix it there instead :)

@alertedsnake
Copy link
Contributor Author

Ah, I know what to do, hang on.

@alertedsnake
Copy link
Contributor Author

I should mention that the lint test runs fine locally...

@toumorokoshi
Copy link
Member

This shows up because of this line here: https://github.com/open-telemetry/opentelemetry-python-contrib/blob/master/exporter/opentelemetry-exporter-datadog/src/opentelemetry/exporter/datadog/exporter.py#L252

I think this is just an incorrect assumption that was made because the default exception handling logic in the SDK is to format the exception that way.

Really exceptions should be recorded as events, so the datadog_exporter should actually use that to extract it instead.

The python code already does this.

I'd recommend for now just modifying the datadog_exporter to not make that assumption, and filing a bug against it so it can be fixed in the future.

What do you think?

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

LGTM! IT would be great if you could file that issue on the dd exporter, but all of your changes are correct and technically the DD issue could occur regardless of merging this change in.

@toumorokoshi
Copy link
Member

toumorokoshi commented Dec 10, 2020

I should mention that the lint test runs fine locally...

regarding this, have you updated your version of PIP? There's a new version that now has strict ordering and also results in detecting conflicting dependencies consistency. I see the linting failing because of install conflicts, at the CI probably updates to the latest version of pip every time.

I see this in the tox lint output:

ERROR: Cannot install opentelemetry-exporter-datadog[test]==0.17.dev0 and opentelemetry-exporter-prometheus-remote-write[test]==0.16.dev0 because these package versions have conflicting dependencies.

The conflict is caused by:
    opentelemetry-exporter-datadog[test] 0.17.dev0 depends on opentelemetry-api==0.17.dev0
    opentelemetry-exporter-prometheus-remote-write[test] 0.16.dev0 depends on opentelemetry-api==0.16.dev0

I think you just need to rebase your commit to fix that. your branch has 16.dev0 as the prometheus writer version. head of the main branch looks to be 0.17.dev0.

@alertedsnake
Copy link
Contributor Author

I think you just need to rebase your commit to fix that. your branch has 16.dev0 as the prometheus writer version. head of the main branch looks to be 0.17.dev0.

Oh! That's a good clue, thanks for catching that, I'll try it.

@alertedsnake
Copy link
Contributor Author

I'd recommend for now just modifying the datadog_exporter to not make that assumption, and filing a bug against it so it can be fixed in the future.

What do you think?

Good call. I've already started working on the Datadog exporter to add metrics (#154), so I'll file an issue and then fix it while I'm in there.

Michael Stella added 9 commits December 10, 2020 09:56
For open-telemetry#139 - some of these things weren't as clear before.
In the real world, the interceptor is always going to be passed a
HandlerCallDetails object with the `method` field set to something
valid... previously these tests set this to an empty string, but that's
not what happens in a real case.

So I've updated the tests to do that right, but also made sure that the
span processing allows this to be unset, just in case someone was
expecting this behavior to be present.
Now checking the span attributes we've added in the Interceptor.

Also fixed some cut & paste errors that resulted in not verifying all
spans as intended.
Added a test for handling gRPC aborts, which was previously missing.

Updated the `net.*` attributes, so that `net.peer.ip` is always set, and
`net.peer.name` is set only if it's `localhost`, since otherwise we'd
need to do DNS lookups, and the spec doesn't require that.

Added tests to verify the `net.*` shows up properly.
It's really nice to have tests to catch this stuff now.
@alertedsnake
Copy link
Contributor Author

Good call. I've already started working on the Datadog exporter to add metrics (#154), so I'll file an issue and then fix it while I'm in there.

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/master/exporter/opentelemetry-exporter-datadog/src/opentelemetry/exporter/datadog/exporter.py#L182

Ooh boy, got my work cut out for me :)

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.

Need Unit Tests for gRPC server instrumentation Make sure rpc instrumentations follow semantic conventions
3 participants