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

[core] Use DEBUG log level for RateSampler initialization #861

Merged
merged 7 commits into from
Apr 9, 2019
Merged

[core] Use DEBUG log level for RateSampler initialization #861

merged 7 commits into from
Apr 9, 2019

Conversation

bmurphey
Copy link
Contributor

In reference to this issue.

This way these logs can be seen when debugging is turned on, but otherwise left out.

This way these logs can be seen when debugging is turned on,
but otherwise left out.
@bmurphey bmurphey requested a review from a team as a code owner March 28, 2019 13:59
gsavovski
gsavovski previously approved these changes Mar 28, 2019
@bmurphey
Copy link
Contributor Author

I've never used tox before, but the celery test that's failing appears to be when trying to test Celery 4.2 with Kombu 4.4. However, Celery 4.2 appears to limit Kombu to >=4.2.0,<4.4. I ran the test locally and got the same issue; then I ran virtually the same test but with Kombu 4.3:

tox -e 'celery_contrib-py27-celery42-redis320-kombu43'

That fails the tests/contrib/celery/test_autopatch.py test. So, I'm not sure what I should do at this point.

@jd
Copy link
Contributor

jd commented Mar 28, 2019

@bmurphey This has been fixed with #858 but it's only in 0.24-dev branch. You should rebase and target this branch AFAIU.

This way these logs can be seen when debugging is turned on,
but otherwise left out.
@bmurphey
Copy link
Contributor Author

Now it's got a tornado test failure in CI, but it works locally (in Docker) with:

tox -e 'tornado_contrib-py34-tornado45'. Unfortunately I don't have permissions (understandably) to try to re-run the test to see if it's just flaky.

@jd
Copy link
Contributor

jd commented Mar 29, 2019

There's a sleep call in the test that fails, so you can bet it's a flaky test. I've re-ran it.

@bmurphey
Copy link
Contributor Author

bmurphey commented Apr 1, 2019

@jd It looks like the test failed again; not sure what I should do here?

@jd jd changed the base branch from master to 0.24-dev April 1, 2019 15:00
@jd
Copy link
Contributor

jd commented Apr 1, 2019

I've switched the target branch to 0.24-dev and updated this PR with the latest commits, let's see.

@bmurphey
Copy link
Contributor Author

bmurphey commented Apr 1, 2019

Looks like a flaky integration test this time 😢

@jd
Copy link
Contributor

jd commented Apr 1, 2019

Yes that's #859 — I've reran the failed job.

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

This is a very reason change. Thanks @bmurphey for this!!!

@brettlangdon
Copy link
Member

Fixes #860

@brettlangdon brettlangdon merged commit deecab3 into DataDog:0.24-dev Apr 9, 2019
@majorgreys majorgreys added this to the 0.24.0 milestone Apr 11, 2019
@majorgreys majorgreys changed the title Use DEBUG log level for RateSampler initialization [core] Use DEBUG log level for RateSampler initialization Apr 11, 2019
@majorgreys majorgreys mentioned this pull request Apr 12, 2019
@bmurphey bmurphey deleted the rate-sampler-log-level branch April 16, 2019 15:35
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.

5 participants