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

integration(kong) Added Kong checks #2241

Closed
wants to merge 1 commit into from

Conversation

shashiranjan84
Copy link

Kong is an open-source management layer for API. More details about Kong can be found at https://getkong.org/

@JohnLZeller
Copy link
Contributor

Hey @shashiranjan84, thanks for the PR!
We'll take a deeper look at this soon.

@irabinovitch
Copy link
Contributor

@shashiranjan84 Thank you for this check and the corresponding integration template. Our team is reviewing and will be back in touch with next steps.

@shashiranjan84
Copy link
Author

Great, thanks.
On Feb 5, 2016 12:17 PM, "Ilan Rabinovitch" notifications@github.com
wrote:

@shashiranjan84 https://github.com/shashiranjan84 Thank you for this
check and the corresponding integration template. Our team is reviewing and
will be back in touch with next steps.


Reply to this email directly or view it on GitHub
#2241 (comment).

@shashiranjan84
Copy link
Author

Any idea when it would be merged and released?

@sonicaghi
Copy link

+1

@irabinovitch
Copy link
Contributor

@shashiranjan84 We merge PRs as we approach a release. At the moment this has the 5.8 label on it.

@shashiranjan84
Copy link
Author

@JohnLZeller @irabinovitch any update on the release date of version 5.8

@degemer degemer self-assigned this May 9, 2016
name, value, tags = row
self.gauge(name, value, tags)
except Exception:
self.log.error(u'Could not submit metric: %s' % repr(row))
Copy link
Member

Choose a reason for hiding this comment

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

Could you do something like this instead : self.log.error(u'Could not submit metric: %s', row) ? Because of https://www.simonmweber.com/2014/11/24/python-logging-traps.html

@degemer
Copy link
Member

degemer commented May 9, 2016

Thanks for your contribution @shashiranjan84 !

I added a few comments, it looks good otherwise (really nice setup for the integration tests).
Once they are addressed, we should be good to merge.

Let me know if you need more information !

@shashiranjan84 shashiranjan84 force-pushed the master branch 2 times, most recently from 4b5979d to 158b142 Compare May 9, 2016 22:31
@shashiranjan84
Copy link
Author

@degemer thanks, updated the code as you advised. Please review.

@shashiranjan84
Copy link
Author

too early, still need to update the test.

@shashiranjan84 shashiranjan84 force-pushed the master branch 2 times, most recently from 80ef990 to f5431d6 Compare May 10, 2016 00:23
@shashiranjan84
Copy link
Author

@degemer I want to update Kong version in the Integration test, so checking if I have some time before next release. I am aiming to finish by tomorrow evening.

@shashiranjan84 shashiranjan84 force-pushed the master branch 6 times, most recently from dbbe42c to d6bd151 Compare May 10, 2016 07:44
@degemer
Copy link
Member

degemer commented May 10, 2016

@shashiranjan84 Yes, today or tomorrow is fine.

About the errors of the CI, the first one is a simple style issue on your Ruby code (https://travis-ci.org/DataDog/dd-agent/jobs/129063985#L319), and the other one is a metrics coverage issue. To fix it, it would be perfect if you could check the presence of all expected metrics and service checks. It's then way easier for us to test the new Kong versions.

The other changes look good !

@shashiranjan84 shashiranjan84 force-pushed the master branch 6 times, most recently from 59060e4 to 3d4944a Compare May 10, 2016 21:35
@shashiranjan84
Copy link
Author

@degemer Please have a look, updated Kong version and test as you advised.


parsed_url = urlparse.urlparse(url)
host = parsed_url.hostname
port = parsed_url.port or 8001
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the default port be 80 in this case ?

Copy link
Author

Choose a reason for hiding this comment

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

Kong default admin port is 8001.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, but if a URL is provided without a specific port, wouldn't requests try the port 80 ? (Since this port is taken from the kong_status_url which is queried afterwards)

Copy link
Author

Choose a reason for hiding this comment

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

should be the parsed_url.port would be null in that case and port will be defaulted 8001?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand what you mean.
I think there are two cases :

  • kong_status_url: http://something:port/status, and parsed_url.port returns port as expected.
  • kong_status_url: http://something/status, and parsed_url.port returns None, and there is (I think) a small issue, because the current check tags with kong_port: 8001 whereas response = requests.get(url, headers=headers(self.agentConfig)) tries to connect on the port 80 (default HTTP port), contradicting the tag.

If you want to have a default for the kong_port, you could instead of asking for an URL in the config file (kong.yaml.example) ask for a port(set a default to 8001) and a host and then build the URL in the check url = 'http://{}:{}/status'.format(host, port).
It would only be possible if the URL is always the same (the /status part).

It's really a nitpick (I guess every user of the check will specify the port), but please let me know what you think about it.

Copy link
Author

@shashiranjan84 shashiranjan84 May 11, 2016

Choose a reason for hiding this comment

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

You are right it will fail while making the request. I have removed the default port and expecting user to always provide the correct kong status url.

@degemer
Copy link
Member

degemer commented May 11, 2016

Thank you ! I tested one last time and noticed two small issues, could you fix them ?

@shashiranjan84
Copy link
Author

@degemer thanks, please review changes.

Kong is an open-source management layer for API. More details about Kong can be found at
website:https://getkong.org/
github:https://github.com/Mashape/kong
@degemer
Copy link
Member

degemer commented May 11, 2016

Merged as 436d0fb.
Thanks again for your contribution and your reactivity !

@degemer degemer closed this May 11, 2016
@shashiranjan84
Copy link
Author

Thank you all your help @dspangen. I have also sent PR for IntegrationTemplate. Please review that too

DataDog/IntegrationTemplate#11

@degemer
Copy link
Member

degemer commented May 11, 2016

@shashiranjan84 Sure, I'll take have a look tomorrow.

@irabinovitch
Copy link
Contributor

@shashiranjan84 we're all set on the integration template. Once the 5.8 agent is released we'll be able to publish the tile and close out that PR.

@sonicaghi
Copy link

@irabinovitch do you know when the 5.8 is going out?

@irabinovitch
Copy link
Contributor

This week.

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.

6 participants