Changes needed for agent-ng - #42
Conversation
|
Hey brendanbenson! Thanks for submitting this pull request! All pull request authors must have a Contributor License Agreement (CLA) on-file with us. Please sign the appropriate CLA (individual or corporate). When sending signed CLA please provide your github username in case of individual CLA or the list of github usernames that can make pull requests on behalf of your organization. |
There was a problem hiding this comment.
What is the point of this change? I could understand something like /[Ii]nsight/, but I'm not sure what the point of the ^p is.
There was a problem hiding this comment.
We renamed insight to p-insight for APM. I don't have full context on the change, but @rmorgan may be able to provide some.
There was a problem hiding this comment.
@rmorgan I thought we'd be searching for the name of the service, which can be anything a users chooses, right? Are we requiring them them to make sure that it has the substring p-insight in it, rather than just insight as it was before? If so, there needs to be an update to the documentation as part of this commit as well.
There was a problem hiding this comment.
p-insight is the name of the service, much like p-mysql is the name of the mysql service provided by Pivotal. I don't have context on what this filter is actually doing, but we shouldn't expect users to name their services with the p- notation. We should be matching any service with the string 'insight'. (assuming this is filtering names within VCAP_SERVICES) I've pushed apps w/ this buildpack fork and named them 'insight' and things seem to work, so maybe that's not what this is doing.
There was a problem hiding this comment.
Then @brendanbenson I'm definitely keen to have this switched back to /insight/, matching the previous (and documented) behavior. Previously there was a problem that you could specify any service that matched /insight/ and it would attempt to use the credentials from those services, regardless of whether it was an Insight service, a Postgresql service, a New Relic service, etc.. Now though, we search a service that both matches /insight/ and has the appropriate credentials (which you updated) which ensures that we're really getting an Insight service.
There was a problem hiding this comment.
Reverted to original behavior. Now complete.
|
In general, this is a higher quality submission than #40, so that's good. There's a couple of changes that need to be made and a couple of questions that need to be addressed. In addition please update the commit message to be more expressive. Take a look at at the commit guildeines section of Pro Git and our Contribution Guidelines for more explanation. |
- Removed code which installed the Insight Analyzer WAR file - Added code to install the cloudfoundry specific Insight weaver instead of the vfabric weaver.
|
@brendanbenson @jcpivotallabs In the interests of getting this issue merged as quickly as possible, I've taken the responsibility of improving the commit message attached to it. Please review a7b0edf before your next submission to see what I will be looking for next time. /cc @rmorgan |
This branches includes the changes needed for the new Spring Insight agent architecture (i.e. removal of analyzer.war)
Ben,
Please ignore the other pull request (#40) you have received... please work off this one.