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

Adds Presto Integration #3131

Merged
merged 20 commits into from
Mar 20, 2019
Merged

Adds Presto Integration #3131

merged 20 commits into from
Mar 20, 2019

Conversation

fanny-jiang
Copy link
Contributor

What does this PR do?

Adds Presto JMX-based integration

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

presto/README.md Outdated Show resolved Hide resolved
presto/README.md Outdated Show resolved Hide resolved
presto/README.md Outdated Show resolved Hide resolved
presto/README.md Outdated Show resolved Hide resolved
presto/README.md Outdated Show resolved Hide resolved
presto/README.md Outdated Show resolved Hide resolved
presto/README.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #3131 into master will decrease coverage by 7.96%.
The diff coverage is 76.47%.

@@            Coverage Diff             @@
##           master    #3131      +/-   ##
==========================================
- Coverage   84.44%   76.47%   -7.97%     
==========================================
  Files         680        4     -676     
  Lines       36099       17   -36082     
  Branches     4195        0    -4195     
==========================================
- Hits        30482       13   -30469     
+ Misses       4382        4    -4378     
+ Partials     1235        0    -1235

Copy link
Contributor

@gzussa gzussa 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 great!!
When running the environment. I am getting this error

ddev env check presto py27-latest 
=========
Collector
=========

  Running Checks
  ==============

    presto
    ------
      Instance ID: presto_380af9d053759530 [ERROR]
      Total Runs: 1
      Metric Samples: Last Run: 0, Total: 0
      Events: Last Run: 0, Total: 0
      Service Checks: Last Run: 0, Total: 0
      Average Execution Time : 0s
      Error: exec: "java": executable file not found in $PATH
      No traceback

Check has run only once, if some metrics are missing you can try again with --check-rate to see any other metric if available.
Note: If some metrics are missing, you may want to try again with the -r / --rate flag.

However, java is in my PATH. Do you know what I am missing?
I didn't check the metadata file in details. However, if there are metrics you are unsure about feel free to ask about them.

presto/manifest.json Outdated Show resolved Hide resolved
presto/manifest.json Show resolved Hide resolved
presto/setup.py Show resolved Hide resolved
presto/tests/conftest.py Show resolved Hide resolved
presto/tests/docker/config/Dockerfile Outdated Show resolved Hide resolved
presto/tox.ini Outdated Show resolved Hide resolved
presto/tests/docker/docker-compose.yaml Outdated Show resolved Hide resolved
presto/tests/test_presto.py Show resolved Hide resolved
@fanny-jiang
Copy link
Contributor Author

fanny-jiang commented Mar 9, 2019

Hey @gzussa! Since java and jmxfetch isn't packaged with the standard agent images, you'll need to start the docker environment using the -jmx tag of agent image. And ddev env check doesn't seem to work with jmx checks, it just hangs. @ofek, @nmuesch, and I were noticing that while testing a few weeks ago. Not exactly sure where the limitation is in the code.

Try ddev env start presto py27-latest -a datadog/agent:latest-jmx

That should start the presto docker environment with the jmx agent. Although we'll may need to fix ddev check to work with jmx checks in order to fully be able to run jmx checks against the docker environment. However, running the agent status command also works fine with checking the status of the presto check:

docker exec -it dd_presto_py27-latest agent status

[...]

========
JMXFetch
========

  Initialized checks
  ==================
    presto
      instance_name : presto-localhost-9999
      message :
      metric_count : 49
      service_check_count : 0
      status : OK
  Failed checks
  =============
    no checks

[...]

@gzussa
Copy link
Contributor

gzussa commented Mar 16, 2019

Maybe, let's add a comment next to the dd_environment method with those details so everyone know about it.

gzussa
gzussa previously approved these changes Mar 20, 2019
nmuesch
nmuesch previously approved these changes Mar 20, 2019
Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

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

LGTM!

Co-Authored-By: nmuesch <nicholas.muesch@datadoghq.com>
@nmuesch nmuesch dismissed stale reviews from gzussa and themself via c859e24 March 20, 2019 14:42
Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

👍 for docs

Copy link
Contributor

@gzussa gzussa 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!

@fanny-jiang fanny-jiang merged commit 0a47b11 into master Mar 20, 2019
@nmuesch nmuesch deleted the fanny/presto branch March 20, 2019 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants