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

Add ability to gather lock metrics #32

Merged
merged 3 commits into from
Jul 30, 2019

Conversation

rabbitt
Copy link
Contributor

@rabbitt rabbitt commented Jul 27, 2019

Description of the changes

Adds the ability to gather database lock metrics. The easiest method for doing so was to leverage the crosstab function of the tablefunc extension. If the extension is not available, a warning will be emitted alerting the user to that fact, and providing instructions on how to enable lock metric gather.

PR Review Checklist

Author

  • add a risk label after carefully considering the "blast radius" of your changes
  • describe the intent of your changes in the description. don't just rewrite your code in prose
  • assign at least one reviewer

Reviewer

  • review code for readability
  • verify that high risk behavior changes are well tested
  • check license for any new external dependency
  • ask questions about anything that isn't clear and obvious
  • approve the PR when you consider it's good to merge

@rabbitt rabbitt force-pushed the feature/lock-metrics branch from f5c2897 to 662af66 Compare July 27, 2019 17:13
@rabbitt
Copy link
Contributor Author

rabbitt commented Jul 28, 2019

@davidbrota Any chance this could get a look over ? :-)

@varas varas requested a review from camdencheek July 29, 2019 09:41
@camdencheek
Copy link
Contributor

@rabbitt Thanks for the PR! I plan to look over this today

@camdencheek
Copy link
Contributor

@rabbitt These changes look great. It might be better behind an argument toggle just so users' logs aren't riddled with warnings for a non-default feature. I can implement that when I update the metrics list. I'll try to get this merged and released tomorrow

@rabbitt
Copy link
Contributor Author

rabbitt commented Jul 29, 2019

@ccheek21 Thanks for the approval! Any idea when this might make it into an official release?

Edit: Apparently you replied as I was writing mine. Yeah, an argument toggle would arguably ('scuse the pun) be a much better mechanism for enabling it - which I'm more than happy to add, if you prefer.

@camdencheek
Copy link
Contributor

@rabbitt If you can add the argument toggle, it'd speed up the release process for me. I plan to release as soon as it gets merged since I don't have any other changes queued right now, so that could be as soon as tomorrow

@rabbitt
Copy link
Contributor Author

rabbitt commented Jul 29, 2019

@ccheek21 any preference on argument name, or is GatherDbLockMetrics fine ?

@camdencheek
Copy link
Contributor

@rabbitt I'd probably go with CollectDbLockMetrics only for consistency with other integrations

@rabbitt
Copy link
Contributor Author

rabbitt commented Jul 29, 2019

Hi @ccheek21,

Let me know if the changes look good, and I can squash the commits down - or if there are additional changes you'd like made.

Thanks!,

--
Carl

@camdencheek
Copy link
Contributor

@rabbitt Looks great -- thanks for the responsive changes! I can take care of squashing and releasing. Apparently there is some work being done on the release pipeline today, so I'm not sure if that will affect a getting the official release out today, but I'll see what I can do

@camdencheek camdencheek merged commit f0b9595 into newrelic:master Jul 30, 2019
@rabbitt rabbitt deleted the feature/lock-metrics branch July 30, 2019 14:11
@camdencheek
Copy link
Contributor

@rabbitt This feature is released with v2.1.3

@rabbitt
Copy link
Contributor Author

rabbitt commented Aug 1, 2019

@ccheek21 I'm working on creating an integration to slurp up Cloudflare's cloudflared metrics (aka their argo tunnel daemon) into newrelic. Once I'm finished, I'd love to get it added to newrelic's suite of ("blessed" ?) integrations. Is that possible? If so, what's the process for that ? Thanks!

@camdencheek
Copy link
Contributor

@rabbitt Thanks for the offer! I'm not entirely sure what that process would look like, but I'm talking with some people who should be able to answer that a bit better. I'll keep you in the loop

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.

2 participants