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.

Sign up for GitHub

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

Jump to bottom

Add search metrics support #989

Merged
merged 1 commit into from
Sep 4, 2018
Merged

Add search metrics support #989

merged 1 commit into from
Sep 4, 2018

Conversation

mkrizek
Copy link
Contributor

@mkrizek mkrizek commented Jul 31, 2018 •
edited
Loading

This adds support for search metrics for Ansible Galaxy. InfluxDB, Grafana and Prometheus containers are added to the dev environment.

Related to #756 and #881.

How to test this:

  1. The PR adds a new requirement in requirements.txt so your dev environment needs to be rebuilt:
    $ make dev/up

  2. This sets up InfluxDB, adds it as a data resource into Grafana and imports dashboard:
    $ make dev/setup-metrics

  3. Search for stuff in Galaxy.

  4. Log into Grafana at http://localhost:3000, login: admin, password: admin.

  5. Navigate to Galaxy Search Metrics - InfluxDB and Galaxy Search Metrics - Prometheus dashboards.

@chouseknecht
Copy link
Contributor

@mkrizek

I'm able to get the DEV environment running with an Influx container. Had to rebuild my galaxy image, in order to install the influxdb package. Once I did that, got my environment running with make dev/up_tmux, followed by make dev/tmux.

Wondering if we should add a grafana container, and maybe focus on building a dashboard in grafana? Do that before we worry about surfacing data back through the Galaxy API. That would allow us to quickly see what metrics are being collected, and maybe we'll better understand how to surface data in Galaxy.

This is looking good!

cutwater
cutwater requested changes Aug 1, 2018
View reviewed changes
galaxy/api/views/search.py Outdated
@@ -33,6 +33,7 @@
from galaxy.api import filters
from galaxy.api import serializers
from galaxy.api.views import base_views as base
from galaxy.common.metricsutils import galaxy_send_metrics
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Maybe calling module just metrics would be more readable?
  2. Please use module import instead of class \ function import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

galaxy/common/metricsutils.py Outdated
logger = logging.getLogger(__name__)


def galaxy_send_metrics(func_name):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't use project name in function or class names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking behind this was to make clear that the decorator is not Django related, but I have zero Django experience, so I'll change it 😄

galaxy/common/metricsutils.py Outdated
return client


def search(request):
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is utility function please don't use request as a parameter. It should be context independent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@mkrizek mkrizek force-pushed the search-metrics branch 2 times, most recently from 173bc4a to 3b18472 Compare August 22, 2018 08:24
cutwater
cutwater approved these changes Aug 22, 2018
View reviewed changes
Copy link
Collaborator

@cutwater cutwater left a comment

Choose a reason for hiding this comment

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

LGTM,

Please when you're ready split it to granular PR's, so that it is easier to review and merge.

@chouseknecht
Copy link
Contributor

chouseknecht commented Aug 23, 2018 •
edited
Loading

@mkrizek

This looks good to me. Really nice job!

IMHO, no need to break this into smaller PRs. I think what you have here fits together nicely, and is not too big.

One last thing, can you please update the CONTRIBUTING.rst file with the following:

  • running the setup piece
  • how to access grafana
  • how to access the dashboards
  • a quick note about how data flows from the app -> prometheus -> influxdb, and how frequently the dashboards will update.

@mkrizek mkrizek changed the title [WIP] Add search metrics support Add search metrics support Aug 24, 2018
@mkrizek
Copy link
Contributor Author

mkrizek commented Aug 24, 2018

@chouseknecht @cutwater ok, CONTRIBUTING.rst updated and commits are squashed.

@chouseknecht chouseknecht mentioned this pull request Aug 27, 2018
Closed
@cutwater cutwater merged commit de0edcc into ansible:devel Sep 4, 2018
@mkrizek mkrizek deleted the search-metrics branch September 4, 2018 11:33
@cutwater cutwater mentioned this pull request Sep 7, 2018
Merged
@newswangerd newswangerd mentioned this pull request Nov 2, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cutwater cutwater cutwater approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mkrizek @chouseknecht @cutwater