This repository has been archived by the owner on Dec 6, 2024. It is now read-only.
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.
Added auto resource detection proposal #111
Added auto resource detection proposal #111
Changes from 2 commits
57178d7
a829129
18155e6
7bbfeae
0eed8a7
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.
What is the context used for here?
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.
This is just a go specific thing, where context always has to be passed around manually. For most other languages, ignore it. I should probably change these snippets to psuedo-code.
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.
The telemetry context (which is indistinguishable from Go's context because it got re-used for that) is very meaningful for OpenTelemetry, so requiring it or not is an important distinction.
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 suggest we remove it from here, as it seems to be a Go-specific thing ;)
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 is a good point and I had not thought about that. I had only included the context in this code snippet for the regular Go usage of context. I don't think there's any requirement to pass the telemetry context into this function, but I'm happy to learn if there is a use case for that.
I removed the code snippets (other than the
Usage
example) & replaced with slightly clearer wording.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 don't believe this will work, since you state that "the resources must be merged in the order the detectors were added". If there were a detector B after the one that failed, call it A, B would have to wait for A's detection to be complete. So there's no real value in making A do detection in a background thread if it blocks B anyways.
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 this could follow the same model we have for vendor exporters, in that they are hosted in a vendor repository.
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 agree that would make sense. Will update based on the outcome of the discussion above.