Skip to content

Remove dogstatsd-python in favor of dogapi - #905

Merged
rocha merged 1 commit into
openedx:masterfrom
rocha:datadog-statsd
Sep 12, 2013
Merged

Remove dogstatsd-python in favor of dogapi#905
rocha merged 1 commit into
openedx:masterfrom
rocha:datadog-statsd

Conversation

@rocha

@rocha rocha commented Sep 9, 2013

Copy link
Copy Markdown
Contributor

Also added datadog application to commom django apps and set it to use
a local statsd server by default.

@rocha

rocha commented Sep 10, 2013

Copy link
Copy Markdown
Contributor Author

@cpennington datadog was not being initialized since our datado django app was not in INSTALLED_APPS. I also changed functions that where using statsd instead of dog_stats_api.

The pearson app is still using DATADOG_API because it uses dog_http_api. Can we change it to use dog_stats_api? @brianhw what do you think?

@brianhw

brianhw commented Sep 10, 2013

Copy link
Copy Markdown
Contributor

Just so I understand, the real fix here is adding datadog to INSTALLED_APPS, so that datadog works at all. (I thought the increment calls were working fine for bulk_email -- just that the timers weren't.) Switching away from dog_http_api is a nice-to-have. If we need to remove dogstats entirely from Pearson, I think that's fine. It could be commented out or replaced. But since you replaced dog_http_api calls elsewhere as part of this PR, is there a reason not to have done Pearson at the same time?

@rocha

rocha commented Sep 10, 2013

Copy link
Copy Markdown
Contributor Author

@brianhw the only module that I found using dog_http_api calls was pearson, using .event. It looks like.increment in dog_stats_api produces the same result as unless dog_http_api.increment. I thought the .event were used for something in particular and wanted to check with you first.

@brianhw

brianhw commented Sep 10, 2013

Copy link
Copy Markdown
Contributor

I don't know too much about .event. Ashley put that in. I think the purpose was to be able to notice when problems occurred with the Pearson feed, as those commands were being called automatically on a schedule.

@cpennington

Copy link
Copy Markdown
Contributor

Actually, .event and .increment mean different things. increment is for a counter, which you'd see as the timeseries part of the datadog chart. events show up as red vertical lines, and I don't think they can be recorded via dog_stats_api.

One other question is whether we should actually switch more in favor of the dogstatsd method of reporting metrics. The advantage there is that it would allow us to switch to a different statsd server as our metrics collection end-point without changing the rest of the code.

@cpennington

Copy link
Copy Markdown
Contributor

@sarina Can you and @rocha coordinate so you aren't conflicting?

@sarina

sarina commented Sep 10, 2013

Copy link
Copy Markdown
Contributor

This doesn't conflict with my branch but it's missing two things that my branch has:

  • Add 'datadog' to cms/envs/common.py
  • Add an __init__.py file to common/djangoapps/datadog (otherwise the imports in the common.py files won't work)

@rocha

rocha commented Sep 10, 2013

Copy link
Copy Markdown
Contributor Author

@cpennington thanks for the clarification.

As for the second part, both dogstatsd and dogapi let you configure the host of the statsd server, and both are compatible with the statsd from etsy, except when using tags. Other python libraries (python-statsd and statsd) also work, but they don't support tags (which we are already using).

It seems to me that dogapi has a better API, mostly because of the context manager for measuring time.

@sarina

sarina commented Sep 10, 2013

Copy link
Copy Markdown
Contributor

👍 by me

Comment thread cms/envs/aws.py

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be nice if this didn't require a synchronous change on the configuration side in production. For instance, you could load in the DATADOG_API key from AUTH_TOKENS as a default value into the DATADOG dictionary (or if the DATADOG dictionary doesn't exist).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like there are still two Pearson commands that reference DATADOG_API. Should these just be changed to use DATADOG instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@cpennington I had that at some point, but end up removing it thinking that it is better to request a change from devops than to provide support for both options. I'll make the change you mention first and request the change, that way we can remove DATADOG_API at some point.

@brianhw I haven't had time to go over the Pearson app yet. I'll ping you when I update it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, that sounds good. We want to be able to remove the old settings, but not require that the settings be updated simultaneously as the code is deployed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Also added datadog application to commom django apps and set it to use
a local statsd server by default.
@rocha

rocha commented Sep 11, 2013

Copy link
Copy Markdown
Contributor Author

@sarina @brianhw updated pearson

@brianhw

brianhw commented Sep 12, 2013

Copy link
Copy Markdown
Contributor

Looks good, as long as tests pass. 👍

rocha added a commit that referenced this pull request Sep 12, 2013
Remove dogstatsd-python in favor of dogapi
@rocha
rocha merged commit 87fcf1a into openedx:master Sep 12, 2013
@rocha
rocha deleted the datadog-statsd branch September 12, 2013 13:54
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Jul 28, 2016
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Sep 16, 2016
…st-playback-status-feature

Add unittest for playback status feature openedx#905 openedx#1039
(cherry picked from commit c6f68cb4df178e0e60133afbcb7f21d11badd720)

 Conflicts:
	biz/djangoapps/ga_achievement/management/commands/tests/test_update_biz_score_status.py
	biz/djangoapps/ga_achievement/tests/factories.py
jcdyer pushed a commit to open-craft/openedx-platform that referenced this pull request Sep 27, 2017
kluo pushed a commit to kluo/edx-platform that referenced this pull request Oct 19, 2019
Commits:
  Send SSO users with existing accounts to /login
DanielVZ96 pushed a commit to open-craft/openedx-platform that referenced this pull request Jan 31, 2024
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