-
Notifications
You must be signed in to change notification settings - Fork 773
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
Publicize required methods to allow for custom implementations of resource detectors. #2949
Publicize required methods to allow for custom implementations of resource detectors. #2949
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2949 +/- ##
==========================================
+ Coverage 83.99% 84.02% +0.03%
==========================================
Files 254 254
Lines 8946 8946
==========================================
+ Hits 7514 7517 +3
+ Misses 1432 1429 -3
|
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.
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.
LGTM.
Will wait to get one more approval before merge, as this is adding new public API very close to a stable release, and want to get one more review.
@@ -27,6 +28,7 @@ public static void Main() | |||
using var tracerProvider = Sdk.CreateTracerProviderBuilder() | |||
.SetSampler(new MySampler()) | |||
.AddSource("OTel.Demo") | |||
.SetResourceBuilder(ResourceBuilder.CreateEmpty().AddDetector(new MyResourceDetector())) |
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.
Not directly related to this PR, this line seems complex, maybe it could be simplified to ResourceBuilder.AddDetector(new MyResourceDetector())
? (by tweaking the static method)
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.
LGTM.
Co-authored-by: Reiley Yang <reyang@microsoft.com>
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.
LGTM
Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com>
…/opentelemetry-dotnet into yunl/ResourceDetectorEx
Changes
Addressed #2897 (comment)
#2897 alone itself was not sufficient for the users to implement their own resource detector. And this PR bridge this gap.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes