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 memcached APM #4281

Merged
merged 3 commits into from
Oct 1, 2019
Merged

Conversation

Matt-Yorkley
Copy link
Contributor

What? Why?

Related to #4256

Enables Memcached APM (currently for UK only) so we can get a deeper look into what's happening with our cache misses.

What should we test?

Dev test only. We need to deploy and check it's working in Datadog.

@Matt-Yorkley Matt-Yorkley added the dev-test A dev need to test this one label Sep 19, 2019
@Matt-Yorkley Matt-Yorkley self-assigned this Sep 19, 2019
@Matt-Yorkley Matt-Yorkley added pr-no-test and removed dev-test A dev need to test this one labels Sep 19, 2019
@kristinalim
Copy link
Member

ok, I tested it with curl on Katuma staging

@Matt-Yorkley Can you share info about how you did this test? From what you said, so I guess the Rails APM is enabled in Katuma staging and this change didn't break the app and Datadog integration?

@kristinalim
Copy link
Member

@Matt-Yorkley Oh, that's why I got confused. You weren't talking about this PR when you mentioned the test with curl. 👍

@sigmundpetersen
Copy link
Contributor

We have a dev-test label now, so this is dev-test not no-test right?

@sigmundpetersen sigmundpetersen added dev-test A dev need to test this one and removed pr-no-test labels Sep 19, 2019
@sauloperez
Copy link
Contributor

I'm having issues testing this. I rebased it to get a db/schema.rb where the latest migrations were run and then successfully deployed but I'm not seeing any data in UK's staging DD APM. Then, I tried provisioning to ensure I was in the latest state but then I get

TASK [datadog : enable datadog memcached integration] *********************************************************************************************************************************
task path: /home/pau/dev/ofn-install/roles/datadog/tasks/memcached_stats.yml:3                                                                                                        
Saturday 21 September 2019  13:25:05 +0200 (0:00:03.891)       0:07:57.680 ****                                                                                                       
fatal: [staging.openfoodnetwork.org.uk]: FAILED! => {"changed": false, "msg": "Could not find or access 'dd-memcached.j2'\nSearched in:\n\t/home/pau/dev/ofn-install/roles/datadog/temp
lates/dd-memcached.j2\n\t/home/pau/dev/ofn-install/roles/datadog/dd-memcached.j2\n\t/home/pau/dev/ofn-install/roles/datadog/tasks/templates/dd-memcached.j2\n\t/home/pau/dev/ofn-instal
l/roles/datadog/tasks/dd-memcached.j2\n\t/home/pau/dev/ofn-install/playbooks/templates/dd-memcached.j2\n\t/home/pau/dev/ofn-install/playbooks/dd-memcached.j2 on the Ansible Controller
.\nIf you are using a module and expect the file to exist on the remote, see the remote_src option"}                                                                                  
        to retry, use: --limit @/home/pau/dev/ofn-install/playbooks/provision.retry

We need to figure this out before moving anything forward potentially broken which might block us later on.

@sauloperez
Copy link
Contributor

sauloperez commented Sep 21, 2019

This gets fixed by openfoodfoundation/ofn-install#526 but then I had to open openfoodfoundation/ofn-install#527 but still no data for staging.

@Matt-Yorkley
Copy link
Contributor Author

Is this good to go now @sauloperez?

@sauloperez
Copy link
Contributor

It still needs to be tested after we merge openfoodfoundation/ofn-install#527

@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented Sep 26, 2019

Ok, I'm going to provision UK staging with openfoodfoundation/ofn-install#527 and then test this.

@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented Sep 26, 2019

@sauloperez I've updated this PR. See addition here: bef4741

I've also tested this on UK staging. I think it's good to go.

@sauloperez
Copy link
Contributor

That last commit implements what was already done in #4295. Which one do you prefer? I think 4295's one is a bit cleaner and any new service we add to the list will have analytics enabled already.

@sauloperez
Copy link
Contributor

Sorry @Matt-Yorkley , I moved on with #4295 to get all these ASAP in production. Once you remove your last commit we'll be good to go.

@sauloperez
Copy link
Contributor

tested and working

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev-test A dev need to test this one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants