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

Instrumentation - Skylight Integration #88

Merged
merged 7 commits into from
Mar 13, 2018

Conversation

hmcfletch
Copy link
Contributor

I took another stab at the Skylight integration. I decided two have an explicit require to turn on the instrumentation instead of using environment variables or messing too much with the core files. This seemed a lot cleaner.

@hmcfletch
Copy link
Contributor Author

A few other notes:

  1. Having some issues with the specs since if the spec for instrumentation gets run before the negative specs get run, then we get failing tests.
  2. Not sure how to test the normalizers (and haven't tested them yet)
  3. Looks like some of the stuff from master got in to this branch...

@wagenet Would love for you to take a look at what I have and see if anything you'd suggest.

Copy link

@wagenet wagenet left a comment

Choose a reason for hiding this comment

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

This looks good to me at a glance. I didn't actually try it out, but I don't see anything problematic about the approach.

@shishirmk
Copy link
Collaborator

Thanks for looking into it @wagenet. @hmcfletch would love to try this out in our internal systems once it is merged to dev.

@hmcfletch
Copy link
Contributor Author

@shishirmk I updated the travis to run the tests separately. Everything is passing now.

.travis.yml Outdated
@@ -6,3 +6,4 @@ rvm:
- 2.5.0
script:
- bundle exec rspec
- bundle exec rspec spec/lib/instrumentation/as_notifications.rb

Choose a reason for hiding this comment

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

If breaking out the spec is the only way to do it, chaining them with && might be preferable (https://docs.travis-ci.com/user/customizing-the-build/#Customizing-the-Build-Step) as it would always generate testing output from both sets but still fail properly.

That said, I think your test tear-down could alias the method back to the original and remove the newly added method easily enough and eliminate the need for the second running of rspec.

@hmcfletch
Copy link
Contributor Author

@GeekOnCoffee Took a crack at it. Hadn't done the remove_method thing before. Pretty nifty.

@shishirmk shishirmk merged commit 3a66a6f into Netflix:dev Mar 13, 2018
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.

4 participants