Azure storage integration #1247
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Integration tests that run azure resources will be skipped if credentials are not available.
Not needed
|
Opened as a draft PR for the moment, as this PR branch is branched from #1225 in order to use the terraform components. |
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪Test errors
Expand to view the tests failures
|
This commit captures sending messages to, and receiving messages from Azure Queue Storage. Move common functionality for working with Terraform and Azure credentials to Elastic.Apm.Test.Utilities.
This commit captures spans for the following Azure storage events - Create container - Delete container - GetBlobs in container - Create page blob - Upload block blob - Upload page blob pages - Download blob - Download streaming blob - Copy from URI into blob - Delete blob
russcam
force-pushed
the
feature/azure-storage
branch
from
c962490
to
d4cc2de
Compare
|
This is ready for review. It instruments the new Azure storage packages, with #1251 as a proposal to instrument the previous Azure storage packages. |
gregkalapos
approved these changes
Apr 7, 2021
src/Elastic.Apm.Azure.Storage/AzureBlobStorageDiagnosticListener.cs
Outdated
Show resolved
Hide resolved
|
|
||
| protected override void HandleOnNext(KeyValuePair<string, object> kv) | ||
| { | ||
| Logger.Trace()?.Log("Called with key: `{DiagnosticEventKey}'", kv.Key); |
There was a problem hiding this comment.
A bit off-topic comment, but maybe we could move this (and the same logs from the other new listeners) into DiagnosticListenerBase - we do this logging in lots of types that derive from DiagnosticListenerBase - we could do this once in the base class.
Doesn't have to be in this PR - this was just a thought when I looked at this.
There was a problem hiding this comment.
agreed, be good to move to the base class. Will open a follow up PR to do so.
| return; | ||
| } | ||
|
|
||
| if (!(kv.Value is Activity activity)) |
There was a problem hiding this comment.
Activity.Id according to the API could be null - it's very hard for me to imagine that, but to be on the safe side I think null-checking for activity.Id would be better, something like this:
Suggested change
| if (!(kv.Value is Activity activity)) | |
| if (!(kv.Value is Activity activity) || string.IsNullOrEmpty(activity.Id)) | |
We have the same in the Elastic.Apm.Azure.ServiceBus and at other places here (also in the OnStop and OnException methods).
Other nice thing about null-checking these is that it'll make Rider code analysis happy (currently it shows this as warning for me).
There was a problem hiding this comment.
An Activity is assigned an id when it's started and Activity.Current validates that the value passed to the setter is either a null value or a started Activity that hasn't finished [1] [2].
Where the Activity is retrieved from the diagnostic event, I think we're safe to assume that the library has started the activity, and where the Activity is retrieved from Activity.Current, I think we can also assume that this is OK because it will either be the activity from the diagnostic event, or an activity started by the agent in case of a transaction.
| { | ||
| case "BlobContainerClient.Create.Start": | ||
| case "PageBlobClient.Create.Start": | ||
| OnStart(kv, "Create"); |
There was a problem hiding this comment.
Any reason we use capital letter here ("Create")? This will end up being the action for the span and I think we use lower case at other places. Definitely won't break anything, just wondering if this is intentional.
There are some other instances in the PR as well.
There was a problem hiding this comment.
Following the convention used for AWS S3 span names (the spec says operation name is camel case, but I think this should read pascal case based on the examples for S3, DynamoDB and the agent weekly conversations). Note that the span action is lowercase.
I don't really have an opinion about which might be best to use for this. Happy to follow up afterwards
test/Elastic.Apm.Azure.Storage.Tests/AzureBlobStorageDiagnosticListenerTests.cs
Show resolved
Hide resolved
|
The one failed test doesn't look related to these changes so merging in. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR provides integrations for Azure storage, using the newer version 12 libraries.
The version 12 libraries is still relatively new, and I suspect that many folks are still using the previous library versions. The new libraries have emit Diagnostic source events for every operation, which is how this integration creates transactions and spans.
The previous libraries do not appear to emit diagnostic events, so would require inspecting HTTP calls to ascertain if the request is to a known service. This could be done as part of this PR, or could be done in a follow up PR, if we agree to support the previous major library versions (different nuget packages).
Closes #1156
Closes #1155