search

Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.05k stars 1.19k forks source link

[@azure/storage-blob] span.context is not a function #16447

Closed nulltoken closed 3 years ago

nulltoken commented 3 years ago

Describe the bug Context: Follow up to #13798

I understand this may not be related to @azure/storage-blob per se (as it look tracing related), but as the issue is triggered by a call to a code similar to the following one, I've figured this repo might be a good starting point.

  const bc = new BlobClient(cn, containerName, blobPath);
  const buf = await bc.downloadToBuffer();

Triggers an exception span.context is not a function

using the following dependencies:

  "dependencies": {
    "@azure/storage-blob": "^12.6.0",
    "@opentelemetry/api": "^1.0.1",
    "@opentelemetry/tracing": "^0.23.0",
    "applicationinsights": "^2.1.4"
  }

Which seems inline with https://github.com/open-telemetry/opentelemetry-js#compatibility-matrix

To Reproduce Steps to reproduce the behavior:

  1. Clone https://github.com/nulltoken/azure-storage-blob-repro-case
  2. edit repro.ts to value the connectionString, the containerName and the path to the blob
  3. yarn install
  4. yarn repro
$ yarn repro
yarn run v1.22.5
$ yarn node -r ts-node/register --unhandled-rejections=strict ./src/repro.ts
ApplicationInsights:An invalid instrumentation key was provided. There may be resulting telemetry loss [ '_your_ikey_' ]
## DEPENDENCY ######################################################
https://github.com/
########################################################
https://github.com/ 236784
## DEPENDENCY ######################################################
https://REDACTED.blob.core.windows.net/REDACTED/REDACTED?se=********************
########################################################
## EXCEPTION ######################################################
span.context is not a function
########################################################

C:\_work\repro\node_modules\@azure\core-http\src\policies\tracingPolicy.ts:79
      const spanContext = span.context();
                               ^
TypeError: span.context is not a function
    at TracingPolicy.<anonymous> (C:\_work\repro\node_modules\@azure\core-http\src\policies\tracingPolicy.ts:79:32)
    at step (C:\_work\repro\node_modules\tslib\tslib.js:143:27)
    at Object.next (C:\_work\repro\node_modules\tslib\tslib.js:124:57)
    at C:\_work\repro\node_modules\tslib\tslib.js:117:75
    at new Promise (<anonymous>)
    at Object.__awaiter (C:\_work\repro\node_modules\tslib\tslib.js:113:16)
    at TracingPolicy.sendRequest (C:\_work\repro\node_modules\@azure\core-http\dist\index.js:4430:22)     
    at StorageClientContext.ServiceClient.sendRequest (C:\_work\repro\node_modules\@azure\core-http\src\serviceClient.ts:303:25)
    at StorageClientContext.<anonymous> (C:\_work\repro\node_modules\@azure\core-http\src\serviceClient.ts:518:34)
    at step (C:\_work\repro\node_modules\tslib\tslib.js:143:27)
error Command failed.
Exit code: 1
Command: C:\Program Files\nodejs\node.exe
Arguments: -r ts-node/register --unhandled-rejections=strict ./src/repro.ts
Directory: C:\_work\repro
Output:

info Visit https://yarnpkg.com/en/docs/cli/node for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Expected behavior No exception ;-) ?

/cc @hectorhdzg

ramya-rao-a commented 3 years ago

@nulltoken Thanks for reporting! Can you share a code snippet showing how you are using @opentelemetry/api, @opentelemetry/tracing and applicationinsights when using the storage blob package?

nulltoken commented 3 years ago

@ramya-rao-a https://github.com/nulltoken/azure-storage-blob-repro-case/blob/main/src/repro.ts is a complete self-contained repro case.

Is that enough?

ramya-rao-a commented 3 years ago

Thanks @nulltoken!

This looks like a case of mismatched dependencies. @azure/storage-blob pulls in a preview version of @azure/core-tracing that depends on 1.0.0-rc.0 of @opentelemetry/api. While you are pulling in the version 1.0.1 of @opentelemetry/api

Can you update your dependency of @opentelemetry/api to 1.0.0-rc.0 and try again?

For a longer term fix, the next storage-blob update will be pulling in the latest of the @opentelemetry/api package.

nulltoken commented 3 years ago

@ramya-rao-a Thanks for the feedback. I'll give it a try then will report back here.

Just a thought, it's a bit complicated to know which version of @azure/storage-blob, @opentelemetry/api, @opentelemetry/tracing, applicationinsights have been tested and proven compatible. It's a little bit like shooting a moving target, in the dark ;-)

Would there be a way to have a compatibility matrix so that updating dependencies becomes a little bit easier?

buerklma commented 3 years ago

Had the same issue after upgrading nestjs to version 8

yarn add @opentelemetry/api@1.0.0-rc.0 did resolve the issue

ramya-rao-a commented 3 years ago

That is a fair point @nulltoken We cannot expect users to look into the dependencies list in the package.json file to find the right version. This ought to have been documented. This becomes even more important now that @opentelemetry/api has gone GA i.e. has a stable release and has more people trying it out.

On the other hand, in our July release, we updated the @azure/core-tracing package to use ^1.0.0 of @opentelemetry/api. So, the packages that had an update then do use the latest @opentelemetry/api. But the catch is that there are still packages that took this change in their beta release or have not made a release yet.

Our August release will play catch up here, but may not cover all the packages. We can make a patch update for such packages to update the readme with a note on the supported version of @opentelemetry/api and @opentelemetry/tracing

@maorleger Thoughts?

nulltoken commented 3 years ago

I'll give it a try then will report back here.

@ramya-rao-a As promised, I downgraded to @opentelemetry/api to 1.0.0-rc.0 and I come back with mixed results:

$ yarn repro
yarn run v1.22.5
$ yarn node -r ts-node/register --unhandled-rejections=strict ./src/repro.ts        
ApplicationInsights:An invalid instrumentation key was provided. There may be resulting telemetry loss [ '_your_ikey_' ]
## DEPENDENCY ######################################################
https://github.com/
########################################################
https://github.com/ 236779
Size of downloaded blob 34866
## DEPENDENCY ######################################################
https://github.com/Azure/azure-sdk-for-js
########################################################
https://github.com/Azure/azure-sdk-for-js 243350
hectorhdzg commented 3 years ago

This expected because ApplicationInsights SDK 2.1.4 is using latest OpenTelemetry and only tracks spans with latest structure, if you downgrade is possible you get issues with Azure SDK packages that were updated previous release and are in fact using latest OpenTelemetry

nulltoken commented 3 years ago

if you downgrade is possible you get issues with Azure SDK packages that were updated previous release and are in fact using latest OpenTelemetry

@hectorhdzg That's also my gut feeling, hence my initial request "Just a thought, it's a bit complicated to know which version of @azure/storage-blob, @opentelemetry/api, @opentelemetry/tracing, applicationinsights have been tested and proven compatible."

drub0y commented 3 years ago

The real key is getting @azure/core-tracing up to the ^1.0.0 version spec on @opentelemetry/api et al. At the time of this writing, @azure/core-tracing's latest version is 1.0.0-preview13 and still references @opentelemetry/api v1.0.0-rc.0 which causes these problems.

The latest versions of Azure SDKs (blob, event grid, etc) all seem to go through the @azure/core-tracing abstractions, so it would seem that if that package is corrected under the covers, all the other Azure SDKs should "just work".

I have spent several hours the past few days digging into this and the best I can do is stop the exception from being thrown, however I lose telemetry for the Azure Event Grid SDK (e.g. I see no dependencies being recorded). If I just install packages as is, I'll actually see a dependency telemetry get generated, but it actually throws an exception right after (context.span one) which breaks the actual sending.

There are some suggestions to "just reference x.y.z version of packages foo and bar yourself" and, ultimately, this is how I at least got the exception to stop being thrown, but you can not overcome the dependencies of the Azure SDKs themselves which will receive their own copies of the incompatible versions they need in their own local node_modules directories.

ramya-rao-a commented 3 years ago

Thanks for your patience everyone

At the time of this writing, @azure/core-tracing's latest version is 1.0.0-preview13 and still references @opentelemetry/api v1.0.0-rc.0 which causes these problems.

Slight correction here: 1.0.0-preview12 and 1.0.0-preview13 of @azure/core-tracing use ^1.0.0 and ^1.0.1 of @opentelemetry/api respectively.

Talking about next steps....

Some of our packages already use 1.0.0-preview12 of @azure/core-tracing and so you should be free to use the latest @opentelemetry/api with them

The below packages will do the same in our August release

Since the storage packages are what is blocking the folks in this thread, we will ship an out of cycle release early next week where they are updated to use the newer @azure/core-tracing package and so can be usable with the latest @opentelemetry/api.

Apart from this, we will look into the compatibility matrix that @nulltoken was suggesting as well as documenting the best practices if you are using applicationinsights. That work is tracked in #15420 which you can subscribe to if you are interested.

drub0y commented 3 years ago

Sorry, you are absolutely correct... my brain is fried after looking at this for days. It's preview.13 that we want, but other dependencies like @azure/core-http and @azure/core-rest-pipeline are looking for preview.11 and preview.12 and that's forcing npm/yarn to pick preview.11 as the main node_modules dependency and I think that ends up creating the dependency issue. I tried to unravel this ball of yarn with explicit dependencies and picking the diff versions of SDK packages, but in my case I've got a mono repo using about 3-4 of the top level @azure/xxx SDK packages and I just cannot find a combination to align them.

Anyway, it's good the problem is known and understood, I'll just have to hold where I am (which is working at least, but missing event grid telemetry) until all the main SDKs are updated and I should be fine.

ramya-rao-a commented 3 years ago

but in my case I've got a mono repo using about 3-4 of the top level @azure/xxx SDK packages and I just cannot find a combination to align them

Can you share what those are?

maorleger commented 3 years ago

@nulltoken thank you for your patience. I believe we are almost caught up, and using your repro repo (thank you for that by the way) I no longer see a runtime exception using @azure/storage-blob@12.7.0

Looks like we're converging correctly on OTel and core-tracing:

validation main % npm ls @azure/core-tracing
validation@1.0.0 /home/mleger/workspace/azure-sdk-experiments/validation
└─┬ @azure/storage-blob@12.7.0
  ├─┬ @azure/core-http@2.1.0
  │ └── @azure/core-tracing@1.0.0-preview.13 deduped
  ├─┬ @azure/core-lro@2.1.0
  │ └── @azure/core-tracing@1.0.0-preview.13 deduped
  └── @azure/core-tracing@1.0.0-preview.13

validation main % npm ls @opentelemetry/api 
validation@1.0.0 /home/mleger/workspace/azure-sdk-experiments/validation
├─┬ @azure/storage-blob@12.7.0
│ └─┬ @azure/core-tracing@1.0.0-preview.13
│   └── @opentelemetry/api@1.0.2 deduped
├── @opentelemetry/api@1.0.2
├─┬ @opentelemetry/tracing@0.23.0
│ ├── @opentelemetry/api@1.0.2 deduped
│ ├─┬ @opentelemetry/core@0.23.0
│ │ └── @opentelemetry/api@1.0.2 deduped
│ └─┬ @opentelemetry/resources@0.23.0
│   └── @opentelemetry/api@1.0.2 deduped
└─┬ applicationinsights@2.1.5
  └── @opentelemetry/api@1.0.2 deduped

Apologies again for the disruption. I know this can be frustrating. We came up with a few paths forward that can help alleviate some of this pain in the future upgrades. Between those and @opentelemetry/api being considered stable we have a path forward to provide an easier migration story when we do these upgrades.

Can you confirm whether things are looking better on your end? Thanks in advance!

maorleger commented 3 years ago

Quick update here: we believe we are all caught up, and haven't heard otherwise. I worked with @hectorhdzg to ensure that applicationinsights@2.1.5 can both generate appInsights metrics and of course does not throw so I think we can close this issue.

Our upgrades going forward will be much smoother thanks to the investments we're making now and since opentelemetry/api has gone GA. So I think we can close this.

nulltoken commented 3 years ago

@maorleger Thanks for the update. I've updated the repro case to the following versions

  "dependencies": {
    "@azure/storage-blob": "^12.7.0",
    "@opentelemetry/api": "1.0.2",
    "@opentelemetry/tracing": "^0.24.0",
    "applicationinsights": "^2.1.5"
  }

And indeed, no crash and the dependency is properly tracked. Thanks to you and @hectorhdzg for this!

Just a minor question: Taking a look at the yarn.lock file, I can still see multiple versions of @azure/core-tracing here (ie. preview12 and preview13). 

$ yarn why @azure/core-tracing
yarn why v1.22.5
[1/4] Why do we have the module "@azure/core-tracing"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "@azure/core-tracing@1.0.0-preview.13"
info Has been hoisted to "@azure/core-tracing"
info Reasons this module exists
   - Hoisted from "@azure#storage-blob#@azure#core-tracing"
   - Hoisted from "@azure#storage-blob#@azure#core-lro#@azure#core-tracing"
info Disk size without dependencies: "124KB"
info Disk size with unique dependencies: "1.43MB"
info Disk size with transitive dependencies: "1.43MB"
info Number of shared dependencies: 2
=> Found "@azure/core-http#@azure/core-tracing@1.0.0-preview.12"
info This module exists because "@azure#storage-blob#@azure#core-http" depends on it.
info Disk size without dependencies: "224KB"
info Disk size with unique dependencies: "1.53MB"
info Disk size with transitive dependencies: "1.53MB"
info Number of shared dependencies: 2
Done in 1.06s.

Is that expected?

Just a thought, it's a bit complicated to know which version of @azure/storage-blob, @opentelemetry/api, @opentelemetry/tracing, applicationinsights have been tested and proven compatible. Would there be a way to have a compatibility matrix so that updating dependencies becomes a little bit easier?

That is a fair point @nulltoken We cannot expect users to look into the dependencies list in the package.json file to find the right version. This ought to have been documented.

@ramya-rao-a Is there an issue I could subscribe to in order to track that topic?

maorleger commented 3 years ago

Just a minor question: Taking a look at the yarn.lock file, I can still see multiple versions of @azure/core-tracing here (ie. preview12 and preview13).

$ yarn why @azure/core-tracing
yarn why v1.22.5
[1/4] Why do we have the module "@azure/core-tracing"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "@azure/core-tracing@1.0.0-preview.13"
info Has been hoisted to "@azure/core-tracing"
info Reasons this module exists
   - Hoisted from "@azure#storage-blob#@azure#core-tracing"
   - Hoisted from "@azure#storage-blob#@azure#core-lro#@azure#core-tracing"
info Disk size without dependencies: "124KB"
info Disk size with unique dependencies: "1.43MB"
info Disk size with transitive dependencies: "1.43MB"
info Number of shared dependencies: 2
=> Found "@azure/core-http#@azure/core-tracing@1.0.0-preview.12"
info This module exists because "@azure#storage-blob#@azure#core-http" depends on it.
info Disk size without dependencies: "224KB"
info Disk size with unique dependencies: "1.53MB"
info Disk size with transitive dependencies: "1.53MB"
info Number of shared dependencies: 2
Done in 1.06s.

Is that expected?

Not expected, but possibly yarn cache or lockfile related? I tried reproducing it on my end but without any success.

For reference, here's what I did:

First experiment:

Second experiment:

Third experiment:

The result for all three tries was the same: 

yarn main % yarn why @azure/core-tracing                                    
yarn why v1.22.5
[1/4] Why do we have the module "@azure/core-tracing"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "@azure/core-tracing@1.0.0-preview.13"
info Reasons this module exists
   - "@azure#storage-blob" depends on it
   - Hoisted from "@azure#storage-blob#@azure#core-tracing"
   - Hoisted from "@azure#storage-blob#@azure#core-http#@azure#core-tracing"
   - Hoisted from "@azure#storage-blob#@azure#core-lro#@azure#core-tracing"
info Disk size without dependencies: "144KB"
info Disk size with unique dependencies: "1.57MB"
info Disk size with transitive dependencies: "1.57MB"
info Number of shared dependencies: 2
Done in 0.18s.

Feel free to let me know if you did something different and I'll try to reproduce it here. Thanks!

maorleger commented 3 years ago

Just a thought, it's a bit complicated to know which version of @azure/storage-blob, @opentelemetry/api, @opentelemetry/tracing, applicationinsights have been tested and proven compatible. Would there be a way to have a compatibility matrix so that updating dependencies becomes a little bit easier?

That is a fair point @nulltoken We cannot expect users to look into the dependencies list in the package.json file to find the right version. This ought to have been documented.

@ramya-rao-a Is there an issue I could subscribe to in order to track that topic?

I created https://github.com/Azure/azure-sdk-for-js/issues/16936 to address this documentation gap. Feel free to subscribe!

nulltoken commented 3 years ago

@maorleger Indeed, it was a case of stale yarn.lock file. Fixed it with a yarn upgrade which bumped @azure/core-http to 2.1.0 and dropped all occurences of preview.12.

Thanks a lot for the tremendous support! ❤️