Skip to content
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

AddHttpClientInstrumentation uses Options<T>. #3051

Merged
merged 3 commits into from
Mar 15, 2022

Conversation

tillig
Copy link
Contributor

@tillig tillig commented Mar 15, 2022

Fixes #3050.

Changes

Copied the pattern from AddAspNetCoreInstrumentation to support IDeferredTracerProviderBuilder when in netstandard. I did notice there's a different options structure for net461 but I didn't modify that code path. I guessed that the full framework folks weren't using Options<T> and, even if they are, I'm on a Mac in VS Code so I can't really exercise that code path myself.

@tillig tillig requested a review from a team March 15, 2022 17:43
@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #3051 (2d39ccc) into main (1c35335) will decrease coverage by 0.19%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3051      +/-   ##
==========================================
- Coverage   84.91%   84.71%   -0.20%     
==========================================
  Files         259      259              
  Lines        9112     9119       +7     
==========================================
- Hits         7737     7725      -12     
- Misses       1375     1394      +19     
Impacted Files Coverage Δ
...umentation.Http/TracerProviderBuilderExtensions.cs 71.42% <63.63%> (-28.58%) ⬇️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 50.00% <0.00%> (-28.58%) ⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 59.09% <0.00%> (-18.19%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 79.41% <0.00%> (-2.95%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 86.75% <0.00%> (-2.74%) ⬇️

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @tillig Would you please update CHANGELOG with a note about this?

PS: Probably the same situation in MeterProviderBuilderExtensions if you also want to update there. But could also be done on a separate PR.

@tillig
Copy link
Contributor Author

tillig commented Mar 15, 2022

Sure, I can do those, too, np.

@tillig
Copy link
Contributor Author

tillig commented Mar 15, 2022

So... possibly slight correction. You don't have an established pattern for how to deal with IDeferredMeterProviderBuilder in any of the *.Instrumentation.* libraries. I see it in the *.Exporter.* libraries, but in instrumentation there's a lot of...

            // TODO: Implement an IDeferredMeterProviderBuilder

            // TODO: Handle HttpClientInstrumentationOptions
            //   SetHttpFlavor - seems like this would be handled by views
            //   Filter - makes sense for metric instrumentation
            //   Enrich - do we want a similar kind of functionality for metrics?
            //   RecordException - probably doesn't make sense for metric instrumentation

Which is to say, there are some design things to figure out. If possible, I'd like to punt that to a different PR so I can at least get the tracing side of things out the door. I will work on the changelog, though.

@cijothomas
Copy link
Member

Which is to say, there are some design things to figure out. If possible, I'd like to punt that to a different PR

Totally agree. Thats the reason why the Extensions.Hosting package never reached stable version. (we had different competing proposals, not nothing was settled, and the focus was mostly on metrics, so the plan was to come back to this after metrics/1.2 stable release...) Happy to get more expert opinions on the topic.

@tillig
Copy link
Contributor Author

tillig commented Mar 15, 2022

Sure. Would it be good to start that maybe as a discussion over here? I have some ideas based on things like how various .NET "builder" things work as well as IOptions<T> handling and more automatic registrations like I talk about in #2971 and #3029. I'll see if I can drop something in there shortly.

@tillig
Copy link
Contributor Author

tillig commented Mar 15, 2022

OK, got CHANGELOG updated.

@cijothomas cijothomas merged commit d1c34ab into open-telemetry:main Mar 15, 2022
@tillig tillig deleted the issue-3050 branch March 15, 2022 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpClientInstrumentationOptions doesn't support IDeferredTracerProviderBuilder
3 participants