search

cloudfoundry / java-buildpack

Cloud Foundry buildpack for running Java applications
Apache License 2.0
439 stars 2.58k forks source link

Azure Application Insights Agent is outdated #980

Closed mayrstefan closed 1 year ago

mayrstefan commented 1 year ago

The buildpack includes an outdated version of Azure Application Insights Agent which was released over two years ago. Version 2.x should be replaced by 3.x We can find an upgrade guide in https://learn.microsoft.com/en-us/azure/azure-monitor/app/java-standalone-upgrade-from-2x I see one breaking change when upgrading the agent. The old version used an instrumentation key which could be set on the command line. The new version changes to a connection string which can be configured as an environment variable or a json file

mayrstefan commented 1 year ago

I found https://learn.microsoft.com/en-us/azure/azure-monitor/app/java-standalone-config#connection-string which states that we still can use a system property applicationinsights.connection.string. So I prepared a PR to make use of a connection_string instead of instrumentation_key in a user-provided-service

dmikusa commented 1 year ago

I saw your related PRs. Thanks for sending them over.

I'm not super familiar with Azure App Insights. I suspect because it's a major version of the agent that if we change the default, we'll end up with the possibility of breaking users. Or are you saying that with the link you referenced users should not notice a difference after upgrading?

The challenge with the Java buildpack and changing defaults is that we should really consider it a breaking change because it can break user applications. Thus we have to either slowly roll out the change, i.e. put in a warning that the change is coming, wait for a while, then make the change.

That or discuss possibly cutting a new major version of the buildpack. I suspect if there are enough different dependencies that we need to bump then perhaps a new major version would make sense. We do also have a PR for Contrast Security. We've also been trying to slow deprecate some code and functionality that has gone EOGS in upstream projects, like Redis session store for Tomcat and the Spring Cloud Connectors and Spring Auto Reconfiguration support. Perhaps it's time to start working towards a new major?

At any rate, the other side of the coin here is that we fully support newer versions of dependencies, they are just not the defaults. In addition, you can easily package your own defaults by packaging your own copy of the buildpack and you can override defaults with the newly added operator config overrides. So if you have an immediate need to see this change, I would suggest looking at one of those options for a short-term fix.

mayrstefan commented 1 year ago

That is why I splitted the changes into two different pull requests. The first one to add the necessary configuration for the 3.x agent (still works for 2.x) and add some initial documentation. We had to reverse engineer how to use it from the source code. Interesstingly current versions are already mirrored into https://java-buildpack.cloudfoundry.org/azure-application-insights/index.yml. At the moment we need to add the UPS to get the java agent added with a fake instrumentation_key which is not used. Instead with have to extend JAVA_OPTS or add an environment variable. This PR makes both versions usable.

The second PR is the change the default version. Because that Agent was not even documented and 2.6.2 is already two years old I'm not sure how many people currently use that agent. So yes, that change would break some users setup (they would need to pin an older version). From a gut feeling I would say 2.x is more or less EOL. How will the buildpack handle deprecated versions of components?

dmikusa commented 1 year ago

That is why I splitted the changes into two different pull requests. The first one to add the necessary configuration for the 3.x agent (still works for 2.x) and add some initial documentation.

OK

At the moment we need to add the UPS to get the java agent added with a fake instrumentation_key which is not used. Instead with have to extend JAVA_OPTS or add an environment variable. This PR makes both versions usable.

Sorry, I'm not 100% sure I follow what you've done here.

I see the PR changes:

  1. Detect will pass if a service has either a connection string or instrumentation key. ✅
  2. Release adds the corresponding system property based on if connection string or instrumentation key is set ✅

That seems good, it'll make things work if the user passes in the correct UPS for the correct version.

What I'm not understanding is how this will help someone that is currently using v2.x and depending on that as the default version. When the buildpack upgrades, they will suddenly get v3.x and their service won't change. It'll still be setting the instrumentation key. Will that be OK? Will the agent still know how to read that?

Also, do you know if the agent will output some sort of warning? Like "Hey, you're using the deprecated instrumentation key. Please switch to using a connection string"? If not, I think the PR should include a warning like that. "WARNING: Your service has provided an Azure App Insights instrumentation key, however, this is deprecated in version 3.x of the Azure App Insigiths agent. Please adjust your service binding.".

mayrstefan commented 1 year ago

What I'm not understanding is how this will help someone that is currently using v2.x and depending on that as the default version. When the buildpack upgrades, they will suddenly get v3.x and their service won't change. It'll still be setting the instrumentation key. Will that be OK? Will the agent still know how to read that?

I don't know either. I'm mostly guessing and trying to understand the code of the agent. This is somehow complicated because the agent mostly references environment variables. There must be some code somewhere that also makes use of the system properties which also work. The initial configuration was in APPLICATION_INSIGHTS_IKEY which got changed to APPINSIGHTS_INSTRUMENTATIONKEY which then got deprecated in favour of APPINSIGHTS_CONNECTION_STRING which seems to include the instrumentation key.

Also, do you know if the agent will output some sort of warning? Like "Hey, you're using the deprecated instrumentation key. Please switch to using a connection string"? If not, I think the PR should include a warning like that. "WARNING: Your service has provided an Azure App Insights instrumentation key, however, this is deprecated in version 3.x of the Azure App Insigiths agent. Please adjust your service binding.".

As far as I could test it 3.4.4 doesn't know about APPLICATION_INSIGHTS_IKEY and complains about a missing connection string. I'll have to retest with APPINSIGHTS_INSTRUMENTATIONKEY which should also work in 2.6

mayrstefan commented 1 year ago

I updated the PR to include the environment variable APPINSIGHTS_INSTRUMENTATIONKEY. That triggers to following warning message

WARN  c.m.a.a.i.c.ConfigurationBuilder - APPINSIGHTS_INSTRUMENTATIONKEY is only supported for backwards compatibility, please consider using APPLICATIONINSIGHTS_CONNECTION_STRING instead

This is a bit misleading because we need to add connection_string key-value to our service but it seems like that makes agent version 3.x work with the provided service

mayrstefan commented 1 year ago

Looks like I was wrong: 2.x is still supported: https://github.com/microsoft/ApplicationInsights-Java/issues/2694#issuecomment-1314525879

mayrstefan commented 1 year ago

Is there a reason why releases 2.6.3 and 2.6.4 are missing in https://java-buildpack.cloudfoundry.org/azure-application-insights/index.yml ?

dmikusa commented 1 year ago

Is there a reason why releases 2.6.3 and 2.6.4 are missing in https://java-buildpack.cloudfoundry.org/azure-application-insights/index.yml ?

I'm not sure. I don't have access to those pipelines anymore. @pivotal-david-osullivan Is the dependency builder pipeline OK for Azure App Insights? Seems like we're missing the latest couple of patches.

pivotal-david-osullivan commented 1 year ago

The pipeline is fine, it is pulling in the latest in the 3.x line. However, the first build for this dependency is for after 2.6.4 was released, when there would have been an already existing 3.x release. It looks like the pipeline that had been pulling 2.x releases was removed/replaced, which might explain why 2.6.3+4 were missed.

dmikusa commented 1 year ago

If we're going to keep 2.x as the default, we should get that pipeline going again. @pivotal-david-osullivan - thoughts on bumping the default?

mayrstefan commented 1 year ago

Any updates on the 2.x pipeline?

pivotal-david-osullivan commented 1 year ago

The 2.6.3 & 2.6.4 releases have been added to the repository so they should be available now. Please leave bumping the default version with me for the moment, I am looking into bumping several other major versions at the same time.

pivotal-david-osullivan commented 1 year ago

@mayrstefan I commented on your PR with an issue I found during testing, but otherwise I'm happy to bump the default version to 3 once the integration still works with the 2.x agent.

mayrstefan commented 1 year ago

Btw.: when testing the PR and switching between versions I got this message when restaging:

[VersionResolver]                WARN  Discarding illegal version 2.4.0-BETA: Invalid micro version '0-BETA'
pivotal-david-osullivan commented 1 year ago

This output is seen because there is an odd BETA release Jar in the Cloudfoundry S3. This has been there for some time but the additions to service detection recently seems to be causing this to print when the service is looked up. I am investigating deleting this Jar which should remove the output.

Closing this as the latest release v4.59.0 bumps the Azure agent to 3.x and still supports 2.x agents. Thanks @mayrstefan!