Remove resource.WithBuiltinDetectors which has not been maintained #2097
Conversation
hanyuancheung
requested review from
Aneurysm9,
dashpole,
evantorrie,
jmacd,
MadVikingGod,
MrAlias,
paivagustavo,
pellared and
XSAM
as code owners
Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #2097 +/- ##
=====================================
Coverage 73.1% 73.1%
=====================================
Files 165 165
Lines 8158 8155 -3
=====================================
- Hits 5966 5965 -1
+ Misses 1963 1961 -2
Partials 229 229
|
|
Some link check has failed, because of the new experimental metrics v0.22.0. But this generally looks good, could anybody answer or help with this? |
sdk/resource/builtin.go
Outdated
| @@ -40,6 +40,7 @@ type ( | |||
| // disable them. | |||
| host struct{} | |||
|
|
|||
| // stringDetector is a Detector that provides information about the | |||
There was a problem hiding this comment.
This is an incomplete sentence. Maybe something got cutoff?
sdk/resource/builtin.go
Outdated
| @@ -78,7 +79,8 @@ func StringDetector(schemaURL string, k attribute.Key, f func() (string, error)) | |||
| return stringDetector{schemaURL: schemaURL, K: k, F: f} | |||
| } | |||
|
|
|||
| // Detect implements Detector. | |||
| // Detect returns a *Resource that describes the string as a value | |||
| // corresponding to k as well as the specific schemaURL. | |||
There was a problem hiding this comment.
What does k refer to here?
There was a problem hiding this comment.
It stands for attribute.Key .
There was a problem hiding this comment.
Please update the comment here to make it clear.
#2106 should resolve the markdown link issue. Updating the branch will pull in the fix. |
…y-go into feature/remove_resource_WithBuiltinDetectors_func
hanyuancheung
force-pushed
the
feature/remove_resource_WithBuiltinDetectors_func
branch
from
0f4c61c
to
0eaa33c
Compare
sdk/resource/process_test.go
Outdated
| t.Run("WithPID", TestWithProcessPID) | ||
| t.Run("WithExecutableName", TestWithProcessExecutableName) | ||
| t.Run("WithExecutablePath", TestWithProcessExecutablePath) | ||
| t.Run("WithCommandArgs", TestWithProcessCommandArgs) | ||
| t.Run("WithOwner", TestWithProcessOwner) | ||
| t.Run("WithRuntimeName", TestWithProcessRuntimeName) | ||
| t.Run("WithRuntimeVersion", TestWithProcessRuntimeVersion) | ||
| t.Run("WithRuntimeDescription", TestWithProcessRuntimeDescription) | ||
| t.Run("WithProcess", TestWithProcess) |
There was a problem hiding this comment.
This runs these tests twice in the CI system, right? Can we remove this and keep the exported functions.
There was a problem hiding this comment.
Already done as mentioned above, please review it again. Thanks
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
|
If everything looks good, please help to approve and merge it. @MrAlias thanks a lot ~ |
hanyuancheung
changed the title
|
Hi, it's a great honor for me to apply for being a member of OpenTelemetry community, after finished reading the membership guidelines doc Here is the list of my contributions to the OpenTelemetry project:
Could anybody please be my sponsor supporting me to do that? |
Resolved: #2026
Remove resource.WithBuiltinDetectors() as well as the test function.