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

2.5.1+ doesn't initialize when used with azure-functions-core-tools #1144

Closed
CreativeTechGuy opened this issue May 14, 2023 · 8 comments · Fixed by #1162
Closed

2.5.1+ doesn't initialize when used with azure-functions-core-tools #1144

CreativeTechGuy opened this issue May 14, 2023 · 8 comments · Fixed by #1162
Assignees

Comments

@CreativeTechGuy
Copy link

TLDR: Two different libraries, applicationinsights and azure-functions-core-tools both overwrite Node.js' require method, thus breaking the magic that azure-functions-core-tools is doing and preventing applicationinsights to hook into the local Azure Function server. This causes automatic logging when running Azure Functions locally to not work and getCorrelationContext() to return null.

This is an incredibly complicated issue. I feel it's best explained by tracing the code since there's a lot going on.

  • The offending commit in this repo is 19d92af. This added a dependency on @azure/opentelemetry-instrumentation-azure-sdk.
  • That dependency depends on @opentelemetry/instrumentation
  • Which that dependency depends on require-in-the-middle, a dependency which overwrites the require method in Node.js. Make a note of this for later.
  • For applicationinsights to add the proper Azure Functions hooks, it checks if @azure/functions-core exists here
  • This @azure/functions-core package doesn't actually exist in NPM or on disk, it is instead provided by azure-functions-core-tools when developing locally. It does this by overwriting the require method in Node.js such that when that specific module is requested, it provides an in-memory object as the response.
  • When installing azure-functions-core-tools it will make a request to install this file as a postinstall script and unpack it into node_modules. The source relevant source code which that zip includes is for the azure-functions-nodejs-worker package.
  • In that worker package, here's the code which overwrites require to supply the fake @azure/functions-core package.

Now, the problem is that there's two packages which are loaded at runtime, both overwriting a global with a custom implementation. azure-functions-core-tools is loaded first so it overwrites first, then require-in-the-middle loads second and overwrites the overwrite. Thus when applicationinsights tries to check for the existence of @azure/functions-core at runtime, it cannot find it. As a result of all of this. our calls to appinsights.getCorrelationContext() return null as it was never initialized as the Azure Function hooks were never setup.

This issue was introduced in 2.5.1 and was not an issue in 2.5.0 of applicationinsights. It's unclear which package is the root cause, it seems like both are equally to blame as they aren't playing nice with each other. Both azure-functions-core-tools and require-in-the-middle "try" to overwrite in a way to be compatible, but due to the use of the Proxy in azure-functions-core-tools it is still incompatible if loaded first.

@CreativeTechGuy CreativeTechGuy changed the title 2,5,1+ doesn't initialize when used with used with azure-functions-core-tools 2.5.1+ doesn't initialize when used with used with azure-functions-core-tools May 14, 2023
@CreativeTechGuy CreativeTechGuy changed the title 2.5.1+ doesn't initialize when used with used with azure-functions-core-tools 2.5.1+ doesn't initialize when used with azure-functions-core-tools May 14, 2023
@JacksonWeber JacksonWeber self-assigned this May 22, 2023
@JacksonWeber
Copy link
Contributor

Hi @CreativeTechGuy, if you want to eliminate the conflict here you could disable Azure SDK auto-instrumentation. This won't fix the core issue that the two dependencies both monkey-patch the require method but might be able to resolve your issue for now. Let me know that workaround helps, thanks!

@CreativeTechGuy
Copy link
Author

@JacksonWeber This doesn't change anything unfortunately. The issue is that the auto-instrumentation doesn't work. So manually disabling the thing that doesn't work doesn't really do anything.

@ejizba
Copy link

ejizba commented May 23, 2023

I took a look and it seems like we're just waiting for the fix to propogate through various dependencies.

  1. require-in-the-middle fixed this in v7.0.0
  2. @opentelemetry/instrumentation updated to require-in-the-middle v7.0.0 in their v0.39.0 release
  3. @azure/opentelemetry-instrumentation-azure-sdk is currently on @opentelemetry/instrumentation v0.38.0 and needs to update to v0.39.0
  4. applicationinsights will need to update to the latest @azure/opentelemetry-instrumentation-azure-sdk

@JacksonWeber @hectorhdzg if you could help speed along those last two updates of packages that would be much appreciated :)

@CreativeTechGuy
Copy link
Author

Thank you @ejizba!

@JacksonWeber
Copy link
Contributor

@ejizba Absolutely. Thank you for noticing the updates in the dependency web there!

JacksonWeber added a commit to Azure/azure-sdk-for-js that referenced this issue May 25, 2023
…entation Packages (#25996)

### Packages impacted by this PR
@azure/monitor-opentelemetry-exporter

### Issues associated with this PR
microsoft/ApplicationInsights-node.js#1144

### Describe the problem that is addressed by this PR
Update the `@opentelemetry/instrumentation` libraries in order to
resolve a dependency conflict with `@azure/functions-core`

### Are there test cases added in this PR? _(If not, why?)_
No. This is just an update of dependencies.

### Checklists
- [x] Added impacted package name to the issue description
- [ ] Does this PR needs any fixes in the SDK Generator?** _(If so,
create an Issue in the
[Autorest/typescript](https://github.com/Azure/autorest.typescript)
repository and link it here)_
- [x] Added a changelog (if necessary)
minhanh-phan pushed a commit to minhanh-phan/azure-sdk-for-js that referenced this issue Jun 12, 2023
…entation Packages (Azure#25996)

### Packages impacted by this PR
@azure/monitor-opentelemetry-exporter

### Issues associated with this PR
microsoft/ApplicationInsights-node.js#1144

### Describe the problem that is addressed by this PR
Update the `@opentelemetry/instrumentation` libraries in order to
resolve a dependency conflict with `@azure/functions-core`

### Are there test cases added in this PR? _(If not, why?)_
No. This is just an update of dependencies.

### Checklists
- [x] Added impacted package name to the issue description
- [ ] Does this PR needs any fixes in the SDK Generator?** _(If so,
create an Issue in the
[Autorest/typescript](https://github.com/Azure/autorest.typescript)
repository and link it here)_
- [x] Added a changelog (if necessary)
@JacksonWeber
Copy link
Contributor

@CreativeTechGuy @ejizba The above issue should be resolved as our dependency on azure/opentelemetry-instrumentation has been updated and that package's dependence on require-in-the-middle and all opentelemetry packages has been updated.

@CreativeTechGuy
Copy link
Author

Is there a plan to release that fix? It looks like it's been committed but the package hasn't been released in a few weeks.

@hectorhdzg
Copy link
Member

@CreativeTechGuy we have a monthly release cadence in our side, we will be publishing a new version of Application Insights Node.js SDK end of next 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 a pull request may close this issue.

4 participants