[core] add flare feature (contact support) #1422
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
What about starting to have a real |
| from checks.check_status import CollectorStatus, DogstatsdStatus, ForwarderStatus | ||
| from config import get_config, get_system_stats, get_parsed_args,\ | ||
| load_check_directory, get_logging_config, check_yaml,\ | ||
| get_config, get_config_path, get_confd_path |
There was a problem hiding this comment.
get_config duped. To avoid that even better:
from config import (
sorted symbols a-z
...
)
|
Info that could be added:
|
| # Save logs file paths | ||
| def _save_logs_path(self, config): | ||
| prefix = '' | ||
| if get_os() == 'windows': |
There was a problem hiding this comment.
Can you use Platform.is_windows() instead?
There was a problem hiding this comment.
Also you should probably use the get_logging_config function https://github.com/DataDog/dd-agent/blob/master/config.py#L900
|
+1 great job overall @degemer ! |
degemer
added a commit
that referenced
this pull request
Mar 12, 2015
- create a directory utils - move configcheck and flare into utils - address various comments #1422
|
Thanks @LeoCavaille & @remh ! |
degemer
added a commit
that referenced
this pull request
Mar 13, 2015
- create a directory utils - move configcheck and flare into utils - address various comments #1422
degemer
added a commit
that referenced
this pull request
Mar 13, 2015
- create a directory utils - move configcheck and flare into utils - address various comments #1422 - add pip freeze output
|
I added the |
degemer
added a commit
that referenced
this pull request
Mar 13, 2015
- create a directory utils - move configcheck and flare into utils - address various comments #1422 - add pip freeze output
| 'hostname': self._hostname, | ||
| 'email': email | ||
| } | ||
| r = requests.post(url, files=files, data=data) |
There was a problem hiding this comment.
Could we pass the apikey to the url as a query string parameter?
|
|
|
||
| # Print output (success/error) of the request | ||
| def _analyse_result(self, resp): | ||
| if resp.status_code == 200: |
There was a problem hiding this comment.
Nitpick but you should likely use:
resp.raise_for_status():It will:
- Make sure that if one day we change the response code from 200 to 202 (which we should), your code would still work
- Format an exception message in a nicer way so you won't have to do it yourself.
There was a problem hiding this comment.
Actually I try to send explicit errors (like 'Apikey unknown' , 'This case is not yours') from the server, which I then catch and display whereas raise_for_status only gives the status code and the standard error associated. I should use ranges for status codes though.
|
Looks almost good! Could you add a test to make sure that the endpoint exist please? |
degemer
force-pushed
the
quentin/flare
branch
2 times, most recently
from
2bfd747 to
9d5a639
Compare
degemer
added a commit
that referenced
this pull request
Mar 17, 2015
- create a directory utils - move configcheck and flare into utils - address various comments #1422 - add pip freeze output
|
I added some test for the |
| # Print output (success/error) of the request | ||
| def _analyse_result(self): | ||
| try: | ||
| json_resp = json.loads(self._resp.text) |
There was a problem hiding this comment.
You coud just do:
json_resp = self._resp.json()
- Create a Flare class to collect all relevant files (logs & configuration files), and then upload them to datadog, which will transfer them to support - Put all code relative to the configcheck in a function for reusability - add the flare option to init.d script
- create a directory utils - move configcheck and flare into utils - address various comments #1422 - add pip freeze output
& rename support URL into something more generic
Create a function in config to get new endpoints, and use it also in the forwarder
- test the upload part (mocking requests.post ) - test the existence of the endpoint (marked as failing for now) - clean configcheck
- add flare option to centos and source install commands - rewrite the analyse_result part (dealing first with our custom 400 errors, then with standard errors, and then with success) - fix endpoint test
|
Awesome! |
remh
added a commit
that referenced
this pull request
Mar 18, 2015
[core] add flare feature (contact support)
|
Sweeeeet! 💯 Also protip @degemer : I did it for you but don't forget to delete your branches once they're merged to master. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
configuration files), and then upload them to datadog, which will
transfer them to support
As I said, it adds some imports and probably belongs somewhere else. (at least to be easily accessible from the GUI for Windows/Mac)