Added resource detectors for AWS services#149
Conversation
Codecov Report
@@ Coverage Diff @@
## main #149 +/- ##
==========================================
- Coverage 79.17% 78.62% -0.55%
==========================================
Files 68 79 +11
Lines 1796 1984 +188
==========================================
+ Hits 1422 1560 +138
- Misses 374 424 +50
|
srprash
left a comment
There was a problem hiding this comment.
Thanks for adding these resource detectors!
We would also need a README for users with instructions and example on using them.
|
Also, the unit test coverage seems to be very low. Can you add more tests to cover the resource detectors? |
| filePath = AWSEBSMetadataWindowsFilePath; | ||
| #endif | ||
|
|
||
| var metadata = this.GetEBSMetadata(filePath); |
There was a problem hiding this comment.
I think we can unit test this by using a mock file system. For instance, if we use the interfaces defined in https://www.nuget.org/packages/System.IO.Abstractions/13.2.47 to interact with the file system, maybe we can add a new utility method where the file system is configurable, and we can pass in a fake file system to mock the responses it gives?
We can also extract all of this logic into a helper method if we need to pass in a new file system for testing, since I know we can't touch the Detect() API.
willarmiros
left a comment
There was a problem hiding this comment.
Added suggestions for unit test coverage
| /// </summary> | ||
| public class ResourceDetectorUtils | ||
| { | ||
| internal static async Task<string> SendOutRequest(string url, string method, KeyValuePair<string, string> header, HttpClientHandler handler = null) |
There was a problem hiding this comment.
This method could also be tested using WireMock or something similar to mock a server, but I think that's probably lower priority given it's just testing the HTTP client.
willarmiros
left a comment
There was a problem hiding this comment.
Let's sync offline on codecov, otherwise looking good!
willarmiros
left a comment
There was a problem hiding this comment.
LGTM! Will wait for @srprash approval
srprash
left a comment
There was a problem hiding this comment.
A minor call out.
Everything else looks good. Thanks for adding in all those tests. :)
PR for adding resource detectors for AWS services.
Currently the OpenTelemetry .NET SDK hasn't expose the
AddDetectorAPI andResourceclass. This is a workaround to provide customers the APIs to opt-in resource detectors with the same user experience. Once the upstream provides the resource detector APIs, will switch to that in future.