nodejs icon indicating copy to clipboard operation
nodejs copied to clipboard

feat: added azure blob instrumentation

Open athirasomanath opened this issue 1 year ago • 7 comments

This feature focuses on instrumenting the classes blobClient and blockBlobClient in azure blob storage: https://instana.kanbanize.com/ctrl_board/56/cards/146704/details/

Methods instrumented for blob:

  • Upload
  • Download
  • Delete

NOTE: Instrumenting BlockBlobClient and BlobClient give easy parsing for proper data required for span creation such as blobname, container name and operation. If using sendOperation, we need to parse it from the url and it doesn't seem neat. Also sendOperation is from @azure/core-http , and even though it is used in almost every operations in storage package, it doesn't act as common point of instrumentation when it comes to a major package like cosmosDB

Things to cover

  • Page Blob class
  • Container Class for instrumenting container functionalities

TODO: Test azure tracing SDK with our otel integration

athirasomanath avatar Dec 13 '23 10:12 athirasomanath

Good Job 🍾

aryamohanan avatar Jan 08 '24 16:01 aryamohanan

Please rebase against main and replace the usage of ProcessControls. The syntax has changed. See example: https://github.com/instana/nodejs/commit/d1b38951f8d8111bdc2cce81aa1a361df4d1fc77#diff-92983afeee93cf42a78a3dd57e5a6e3e96662d66c7f7c6954548e0723322c601L34

kirrg001 avatar Feb 14 '24 11:02 kirrg001

Hey!

I need to do another review iteration with fresh eyes. Please take a look at my first comments 👍

Here are some additional findings:

  • ESM app is missing. Please add an ESM app.mjs and check if azure blob storage works with ESM - see.
  • I could not find the ticket reference in the PR nor in the commits.
  • All of your files did not run through code formatting - please do.

Reply to this: Addressed the Issues

athirasomanath avatar Feb 22 '24 07:02 athirasomanath

Thank you @aryamohanan and me will do another review 👍

kirrg001 avatar Feb 22 '24 12:02 kirrg001

The plan is to review and merge this on Monday or better said: after the next release.

kirrg001 avatar Feb 22 '24 14:02 kirrg001

Pushed a commit to run all tests using [ci all-tests]. Waiting for success result.

kirrg001 avatar Feb 23 '24 11:02 kirrg001

Passed 👍

kirrg001 avatar Feb 23 '24 11:02 kirrg001