-
Notifications
You must be signed in to change notification settings - Fork 750
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 & use ConfigureResource API. #3307
Add & use ConfigureResource API. #3307
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3307 +/- ##
==========================================
+ Coverage 86.20% 86.25% +0.05%
==========================================
Files 258 258
Lines 9254 9269 +15
==========================================
+ Hits 7977 7995 +18
+ Misses 1277 1274 -3
|
@Oberon00 Thanks for the PR. I am going to hold off for a week, as we'll be doing 1.3.0 stable very soon, and want to include this to 1.4 series, so as to have some time to gather feedbacks before calling it stable. |
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 like this PR @Oberon00, big improvement! Just have one comment to discuss naming, but otherwise things seem solid.
options.SetResourceBuilder(ResourceBuilder.CreateDefault().AddService( | ||
serviceName: "MyService", | ||
serviceVersion: "1.0.0")); | ||
options.ConfigureResources(r => r.AddService(serviceName: "MyService", serviceVersion: "1.0.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'll need to make sure we follow up with opentelemetry.io docs as well. Quite a few examples using SetResourceBuilder
like: https://opentelemetry.io/docs/instrumentation/net/getting-started/#console-application
@Oberon00 could you fix the conflicts? |
Make sure to change the name (or let me change the name) before releasing if desired. |
Build failures seem unrelated to this PR. |
@Oberon00 you can close/open the PR (or push a dummy commit) to re-trigger CI. |
D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry.publicApi\net462\PublicAPI.Unshipped.txt(2,1): error RS0017: Symbol 'OpenTelemetry.Logs.OpenTelemetryLoggerOptions.ConfigureResources(System.Action<OpenTelemetry.Resources.ResourceBuilder!>! configure) -> OpenTelemetry.Logs.OpenTelemetryLoggerOptions!' is part of the declared API, but is either not public or could not be found [D:\a\opentelemetry-dotnet\opentelemetry-dotnet\src\OpenTelemetry\OpenTelemetry.csproj] ^^ This looks like this PR itself. Could you check this? |
1834277
to
7d7054d
Compare
7d7054d
to
d4ba187
Compare
Hopefully I now covered all target frameworks. |
/// <param name="tracerProviderBuilder">TracerProviderBuilder instance.</param> | ||
/// <param name="configure">An action which modifies the provided <see cref="ResourceBuilder"/> in-place.</param> | ||
/// <returns>Returns <see cref="TracerProviderBuilder"/> for chaining.</returns> | ||
public static TracerProviderBuilder ConfigureResource(this TracerProviderBuilder tracerProviderBuilder, Action<ResourceBuilder> configure) |
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.
Staged up the change in #3411. I'm kind of partial to the shorter name, but open to the name change if others like it.
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.
Looks good overall. I would like to get more thoughts on ConfigureResource
vs ConfigureResourceBuilder
as the name, but this can be decided before we ship a stable version, so wont block this PR.
* Added Jaeger Propagator to Opentelemetry.Extensions.Propagators (#3309) * Remove unnecessary bullet in CHANGELOG.md (#3352) Co-authored-by: Cijo Thomas <cithomas@microsoft.com> * Fix OTLP test (#3357) * Show that test is not doing what you might think it does * More asserts the merrier * Show this little test that it has potential * improve test coverage: InMemoryExporter & IDeferredMeterProviderBuilder (#3345) * [SDK] Circular buffer tweaks + cpu pressure test (#3349) * CircularBuffer tweaks and cpu pressure test. * Switch to Volatile.Read. * Perf tweaks. * Remove race check in debug after doing more testing. Co-authored-by: Cijo Thomas <cithomas@microsoft.com> * Fix event name logic + support null categoryname. (#3359) * In-memory Exporter: Buffer log scopes (#3360) * Buffer log scopes when using in-memory exporter. * CHANGELOG update. * Code review. * Tests. * CHANGELOG tweak. * SDK: Forward SetParentProvider to children of CompositeProcessor (#3368) * Examples: Fix ParentProvider not being set on MyFilteringProcessor (#3370) * Fix ParentProvider not being set on MyFilteringProcessor example. * Added XML comments. * Tweak. * Typo. * Logs: Add helper ctors & forceflush on OpenTelemetryLoggerProvider (#3364) * Add helper ctors & forceflush on OpenTelemetryLoggerProvider. * CHANGELOG update. * Unit tests. * Code review. * Code review. * Tweak. * SDK: Nullable annotations for base classes & batch + shims to enable compiler features (#3374) * Nullable annotations and shims for SDK base classes & batch. * Target updates. * Remove System.Collections.Immutable ref. * ApiCompat attribute exclusions. * ASPNETCore instrumentation to populate httpflavor tag (#3372) * improve test coverage: InMemoryExporter SnapshotMetric (#3344) * Fix AspNetCore metrics to use correct value for http.flavor (#3379) * Fix AspNetCore metrics to use correct value for http.flavor * word better * Logs: Add LogRecordData (#3378) * Add LogRecordData and hook up to LogRecord. * CHANGELOG update. * Code review. * Remove SetHttpFlavor from Http instrumentations (#3381) * Asp.Net Core trace instrumentation to populate http schema tag (#3392) * Try asp.net core tests with inproc server (#3394) * Dedupe IsPackable (#3398) * Remove AspNet and AspNet.TelemetryHttpModule instrumentation projects (#3397) * Handle possible exception when initializing the default service name (#3405) * HttpClient: Invoke Enrich when SocketException = HostNotFound (#3407) * Add & use ConfigureResource API. (#3307) Co-authored-by: tyler jago <ty_bone11@hotmail.com> Co-authored-by: Robert Pająk <rpajak@splunk.com> Co-authored-by: Cijo Thomas <cithomas@microsoft.com> Co-authored-by: Timothy Mothra <tilee@microsoft.com> Co-authored-by: Mikel Blanchard <mblanchard@macrosssoftware.com> Co-authored-by: Reiley Yang <reyang@microsoft.com> Co-authored-by: Utkarsh Umesan Pillai <utpilla@microsoft.com> Co-authored-by: Christian Neumüller <christian.neumueller@dynatrace.com>
Fixes #2909.
Changes
Added ConfigureResources which can replace SetResourceBuilder more succinctly in most cases
and has greater flexibility (applies to TracerProvicerBuilder, MeterProviderBuilder, OpenTelemetryLoggingOptions).
CHANGELOG.md
updated for non-trivial changes[ ] Design discussion issue #Although this is very similar to the design proposed in SetResourceBuilder is overly restrictive, no way to cooperatively initialize Resources #2909