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 presto tests, add to CI, update integration details #975

Merged

Conversation

ericmustin
Copy link
Contributor

@ericmustin ericmustin commented Mar 17, 2020

This PR adds Presto Tests to the CI properly this time, as well as add a fix and tests for analytics, and lastly addresses an issue where the ensure block not having an implicit return was causing some specs to fail.

According to rubygems the min ruby version here for presto-client is 1.9.1 (see below), so I've gone back through the rake tasks and ensured that presto tests run for all versions currently supported.

  • versioning note while the listed min version is 1.9.1, originally the 2.0 specs were failing. I looked into this and in 0.5.13 they added the usage of Process::CLOCK_MONOTONIC (changelog) which is only supported in >=ruby2.1. They've removed support for 2.0 and lower (commit), and given that dd-trace-rb only supports presto-client >= 0.5.14 , I've removed the presto tests from all ruby 2.0 related tests in the ci, rakefile, and docker-compose

Additionally, it looks like some last minute tinkering with variable names for datadog_configuration was creating some issues with setting span tags, as the analytics sampling block was throwing an error which caused the span's resource name not to get set properly. This has been fixed, not sure how that slipped through my development testing, but was compounded by the fact that the presto tests weren't added to the circleci config properly.

I believe I added the mock presto container to the circleci config properly but making this PR should validate or surface any issues, as it's a bit hard to test this locally.

Thanks for the patience here

@ericmustin ericmustin requested a review from a team March 17, 2020 10:51
@marcotc marcotc added bug Involves a bug integrations Involves tracing integrations labels Mar 17, 2020
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

🎆 Great work @ericmustin, thank you for the quick turnaround on this follow up PR.

@ericmustin ericmustin merged commit 5eec549 into DataDog:master Mar 17, 2020
@ericmustin ericmustin added this to Merged & awaiting release in Active work via automation Mar 17, 2020
@marcotc marcotc mentioned this pull request Mar 17, 2020
@delner delner added this to the 0.34.0 milestone Mar 30, 2020
@delner delner moved this from Merged & awaiting release to Released in Active work Mar 31, 2020
@ericmustin ericmustin deleted the fix_presto_tests_and_integration branch August 10, 2020 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug integrations Involves tracing integrations
Projects
Active work
  
Released
Development

Successfully merging this pull request may close these issues.

None yet

3 participants