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.
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
Add AWS resource detectors to extension package #586
Add AWS resource detectors to extension package #586
Changes from all commits
50a280c
14f450a
0abe18b
7b8cf1b
2f919b3
36dd476
7bee607
2b04578
a72190a
f11c96f
cf539d2
6a1caaa
7a73524
0583ed4
e7f39ae
49790cf
5533fa1
5e2e67a
d9762e6
549b9e2
25a904a
3ada952
4756278
aeb838f
9bd36f8
cf0efc4
e63224e
24e8326
2e8d7fe
87863eb
d6ad808
480c112
58445b8
9cd22df
530a91b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
opentelemetry.sdk.extension.aws.resources
plural to matchopentelemetry.sdk.resources
? Not a big deal though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't even notice that, actually I did
resource
because that is the folder it is in on the specificationsAlso in java we put it under resource
And yet JavaScript, Go and PHP all put it under a folder called
detectors/
😅I guess I'll defer to the SIG here, as GCP and other vendors will probably add their resource Detectors soon... maybe we should even move the SDK import path? 😓 Although I imagine that will be hard after 1.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't at
1.0
yet so nows the time to decide on an import path :D. But again, not a blocker.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I meant we should move
opentelemetry.sdk.resources
toopentelemetry.sdk.resource
to match the spec 🙂I actually like
.resource
because there is only 1resource:
namespace in the attributes. Even if you add multipleResourceDetector
s you're still setting attributes in 1 namespace...I'll leave it for now and if we need to we'll change it before this package goes 1.0 😄. Shouldn't be a big deal to do "both" if we need to as well later!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm it would have been nice if this was handled in the
ResourceDetector
super class so the implementations didn't have to duplicate it every time, but too late now I think 🤷♂️There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a really good point, do you mean something like this on opentelemetry-python-core?:
I think that would definitely be worth it, maybe we can add it in a minor release for now and support (but deprecate) the existing method?
If this sounds like it makes sense I can open a small PR with this change on core! 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'll be better to catch this specific type of exception below than to have nested general exceptions caught.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand you correctly, you think it would be better than we catch
FileNotFoundError
specifically? That makes sense to me! I'll update it here and in theeks.py
file.