Conditional at Launch #32

nebhale · 2020-11-13T21:40:21Z

Previously, a binding was required at build time in order to contribute the layers for the Application Insights components. We've started to think that requiring a binding during build is too onerous a restriction and instead environment variables should be used to indicate that a dependency should be contributed. The knock-on effect of this change is that we have to move a lot more of the conditional logic out to launch time including ensuring that if the binding doesn't exist nothing is contributed. This change makes those updates to the buildpack's behavior.

In additional, this includes a fix where the provided build plan wasn't exhaustive for combinations that a user need when using the buildpack.

ekcasey

I avoid repeating questions/comments from my review of paketo-buildpacks/google-stackdriver#38, assuming whatever the resolution is there we will make this PR consistent.

Reading docs for https://docs.microsoft.com/en-us/azure/virtual-machines/windows/instance-metadata-service raised an additional consideration. I think in both this and the google-stackdriver PR we should be appending the metadata server host to the no_proxy and NO_PROXY environment variables at build time. The agents probably skip the proxy for the metadata service by default but our InAzure and InGcp functions won't unless we configure it.

README.md

helper/nodejs_test.go

Previously, a binding was required at build time in order to contribute the layers for the Application Insights components. We've started to think that requiring a binding during build is too onerous a restriction and instead environment variables should be used to indicate that a dependency should be contributed. The knock-on effect of this change is that we have to move a lot more of the conditional logic out to launch time including ensuring that if the binding doesn't exist _nothing_ is contributed. This change makes those updates to the buildpack's behavior. In additional, this includes a fix where the provided build plan wasn't exhaustive for combinations that a user need when using the buildpack. Signed-off-by: Ben Hale <[email protected]>

ekcasey

Again, I didn't repeat comments that I already made on paketo-buildpacks/google-stackdriver#38, I figured that if you accepted any of those suggestions we would make this implementation consistent.

ekcasey · 2020-11-20T19:24:20Z

README.md

-# `gcr.io/paketo-buildpacks/azure-application-insights`
-The Paketo Azure Application Insights Buildpack is a Cloud Native Buildpack that contributes the Application Insights Agent and configures it to connect to the service.
+# `paketo-buildpacks/microsoft-azure`
+The Paketo Microsoft Azure Buildpack is a Cloud Native Buildpack that contributes Microsoft Azure agents and configures them to connec to their services.


Suggested change

The Paketo Microsoft Azure Buildpack is a Cloud Native Buildpack that contributes Microsoft Azure agents and configures them to connec to their services.

The Paketo Microsoft Azure Buildpack is a Cloud Native Buildpack that contributes Microsoft Azure agents and configures them to connect to their services.

ekcasey · 2020-11-20T19:34:45Z

README.md

+### Type: `MicrosoftAzure`
+|Key                  | Value   | Description
+|---------------------|---------|------------
+|`InstrumentationKey` | `<key>` | Azure Application Insights instrumentation key


ekcasey · 2020-11-20T19:43:07Z

README.md

 ### Type: `dependency-mapping`
 |Key                   | Value   | Description
 |----------------------|---------|------------
 |`<dependency-digest>` | `<uri>` | If needed, the buildpack will fetch the dependency with digest `<dependency-digest>` from `<uri>`

+## License
+This buildpack is released under version 2.0 of the [Apache License][a].
+
 [a]: http://www.apache.org/licenses/LICENSE-2.0


Suggested change

[a]: http://www.apache.org/licenses/LICENSE-2.0

[a]: http://www.apache.org/licenses/LICENSE-2.0

[i]: https://docs.microsoft.com/en-us/azure/azure-monitor/app/create-new-resource#copy-the-instrumentation-key

ekcasey · 2020-11-20T19:52:54Z

appinsights/java.go

@@ -52,7 +56,8 @@ func (j JavaAgent) Contribute(layer libcnb.Layer) (libcnb.Layer, error) {
 		if err := sherpa.CopyFile(artifact, file); err != nil {
 			return libcnb.Layer{}, fmt.Errorf("unable to copy %s to %s\n%w", artifact.Name(), file, err)
 		}
-		layer.LaunchEnvironment.Appendf("JAVA_TOOL_OPTIONS", " ", "-javaagent:%s", file)
+
+		layer.LaunchEnvironment.Default(AgentPath, file)

 		file = filepath.Join(j.BuildpackPath, "resources", "AI-Agent.xml")


Not relevant to this PR but should we create an issue to provide build and or launch time configuration options that allow folks to change the settings in AI-Agent.xml? I imagine this is something App Insights users may want eventually.

ekcasey · 2020-11-20T20:12:06Z

appinsights/nodejs.go

+
+const (
+	NodeModule = "applicationinsights"
+	NodePath   = "BPI_AZURE_APPLICATION_INSIGHTS_NODE_PATH"


Suggested change

NodePath = "BPI_AZURE_APPLICATION_INSIGHTS_NODE_PATH"

AppInsightsNodePath = "BPI_AZURE_APPLICATION_INSIGHTS_NODE_PATH"

I suggest making this more specific only because, when I saw the variable used below, I assumed we were setting a default for NODE_PATH and was momentarily confused.

ekcasey · 2020-11-20T20:25:59Z

cmd/helper/main.go

+		} else if g, ok, err := bindings.ResolveOne(bs, bindings.OfType("MicrosoftAzure")); err != nil {
+			return fmt.Errorf("unable to resolve MicrosoftAzure binding\n%w", err)
+		} else if ok {
+			b = g


Suggested change

} else if g, ok, err := bindings.ResolveOne(bs, bindings.OfType("MicrosoftAzure")); err != nil {

return fmt.Errorf("unable to resolve MicrosoftAzure binding\n%w", err)

} else if ok {

b = g

} else if m, ok, err := bindings.ResolveOne(bs, bindings.OfType("MicrosoftAzure")); err != nil {

return fmt.Errorf("unable to resolve MicrosoftAzure binding\n%w", err)

} else if ok {

b = m

assume the g is a copy pasted shorthand for google?

ekcasey · 2020-11-20T20:38:27Z

credentials/credentials.go

-		e[fmt.Sprintf("APPINSIGHTS_%s", s)] = v
+	if s, ok := l.Binding.Secret["InstrumentationKey"]; ok {
+		l.Logger.Info("Configuring Azure Application Insights instrumentation key")
+		return map[string]string{"APPINSIGHTS_INSTRUMENTATIONKEY": s}, nil
 	}


Do we potentially want to fail or warn if there is no InstrumentationKey in the binding?

ekcasey · 2020-11-20T20:41:22Z

internal/nodejs/module_test.go

+	context("RequireModule", func() {
+
+		it("renders require module", func() {


Suggested change

context("RequireModule", func() {

it("renders require module", func() {

context("RequireModule", func() {

it("renders require module", func() {

nebhale added semver:major A change requiring a major version bump type:enhancement A general enhancement labels Nov 13, 2020

nebhale requested a review from ekcasey November 13, 2020 21:40

ekcasey reviewed Nov 16, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

helper/nodejs_test.go Outdated Show resolved Hide resolved

ekcasey mentioned this pull request Nov 16, 2020

Conditional at Launch paketo-buildpacks/google-stackdriver#38

Draft

nebhale force-pushed the conditional-at-launch branch 3 times, most recently from e807a20 to a0e3e31 Compare November 18, 2020 00:53

nebhale requested a review from ekcasey November 18, 2020 00:53

nebhale force-pushed the conditional-at-launch branch from a0e3e31 to 2881082 Compare November 18, 2020 01:16

ekcasey reviewed Nov 20, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditional at Launch #32

Conditional at Launch #32

nebhale commented Nov 13, 2020

ekcasey left a comment

ekcasey left a comment

ekcasey Nov 20, 2020

ekcasey Nov 20, 2020

ekcasey Nov 20, 2020

ekcasey Nov 20, 2020

ekcasey Nov 20, 2020

ekcasey Nov 20, 2020

ekcasey Nov 20, 2020

ekcasey Nov 20, 2020

	The Paketo Microsoft Azure Buildpack is a Cloud Native Buildpack that contributes Microsoft Azure agents and configures them to connec to their services.
	The Paketo Microsoft Azure Buildpack is a Cloud Native Buildpack that contributes Microsoft Azure agents and configures them to connect to their services.

	\|`InstrumentationKey` \| `<key>` \| Azure Application Insights instrumentation key
	\|`InstrumentationKey` \| `<key>` \| Azure Application Insights [instrumentation key][i]

	[a]: http://www.apache.org/licenses/LICENSE-2.0
	[a]: http://www.apache.org/licenses/LICENSE-2.0
	[i]: https://docs.microsoft.com/en-us/azure/azure-monitor/app/create-new-resource#copy-the-instrumentation-key

	NodePath = "BPI_AZURE_APPLICATION_INSIGHTS_NODE_PATH"
	AppInsightsNodePath = "BPI_AZURE_APPLICATION_INSIGHTS_NODE_PATH"

		context("RequireModule", func() {

		it("renders require module", func() {

Conditional at Launch #32

Are you sure you want to change the base?

Conditional at Launch #32

Conversation

nebhale commented Nov 13, 2020

ekcasey left a comment

Choose a reason for hiding this comment

ekcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment