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

[apmazure] add blob storage #1105

Merged
merged 25 commits into from
Aug 24, 2021
Merged

Conversation

stuartnelson3
Copy link
Contributor

Add azure blob storage instrumentation.

I added instrumentation for azurequeue and moved the code into the interface seen in this PR, and then realized I was using the wrong azure lib. I decided to just bring over the interface from the other code for now.

There are still 3 other azure storage services to implement before #885 is officially "done":

  • Table
  • File Storage
  • Queue

Structure is:

<Storage Account Name>.blob.core.windows.net

We could parse it to get storage-account-name +
service (blob, in this case), but the information
also seems to be available in the path of the
constructed url.
This reverts commit 33bc894.
Some of the azure go sdk modules require go1.14
go vet fails when run on a directory with no
buildable files, eg. running go vet with a version
of go too low for the provided dir.
solves the problem when we are attempting to go
vet a module, but the version of go we are using
isn't supported.
@stuartnelson3 stuartnelson3 requested a review from a team August 19, 2021 09:15
@apmmachine
Copy link
Contributor

apmmachine commented Aug 19, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-23T10:13:30.707+0000

  • Duration: 31 min 59 sec

  • Commit: ad0ff51

Test stats 🧪

Test Results
Failed 0
Passed 10922
Skipped 268
Total 11190

Trends 🧪

Image of Build Times

Image of Tests

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a handful of minor things. I think we should probably use httptest.Server in the tests, but I don't know the details.

module/apmazure/storage.go Show resolved Hide resolved
module/apmazure/storage.go Outdated Show resolved Hide resolved
module/apmazure/storage.go Outdated Show resolved Hide resolved
module/apmazure/storage.go Outdated Show resolved Hide resolved
module/apmazure/storage.go Outdated Show resolved Hide resolved
module/apmazure/storage.go Outdated Show resolved Hide resolved
Comment on lines 60 to 61
// TODO: If we use a fake URL, the test is fast but we do not set a
// status code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we set a status code? can we not use an httptest server?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the request errors, resp.Response() is nil, so we can't get a status code. We could use something like example.blob.core.windows.net and get a 4XX response, but then the test takes a while longer (~1sec).

If we use httptest.Server, the URL is a local IP addr, but we rely on the subdomains in the provided url to get the account name and storage type (blob, queue, eg): https://fakeaccnt.blob.core.windows.net. We could use an httptest server and check the status code is set correctly, but then we won't be able to parse out (in this case) fakeaccnt and blob for other span properties.

Copy link
Member

@axw axw Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. What about setting PipelineOptions.HTTPSender, and skipping the net/http stack altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea!

@stuartnelson3 stuartnelson3 merged commit 2a0dae9 into elastic:master Aug 24, 2021
@stuartnelson3 stuartnelson3 deleted the azure-storage branch August 24, 2021 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants