-
Notifications
You must be signed in to change notification settings - Fork 759
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
Hosting: README documentation + tweaks #2080
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2080 +/- ##
==========================================
+ Coverage 82.12% 84.99% +2.87%
==========================================
Files 250 187 -63
Lines 6774 6071 -703
==========================================
- Hits 5563 5160 -403
+ Misses 1211 911 -300
|
.AddAspNetCoreInstrumentation() | ||
.AddHttpClientInstrumentation() | ||
.AddZipkinExporter() | ||
.AddMyFeature()); |
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.
Does this mean ZipkinExporter will run before MyProcessor?
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.
Does this mean ZipkinExporter will run before MyProcessor?
Yes.
I didn't think of this until right now, but this is a kind of interesting situation...
.AddMyFeature()
.AddProcessor(new CustomProcessor())
In that case CustomProcessor will run before MyFeature's MyProcessor because MyFeature is using Configure
which defers execution (AddProcessor call) until Build is called.
May or may not be an issue?
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.
PS: I switched the order in the README to make it more real-world.
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 an issue in my opinion, but some folks might get shocked if they were expecting different behavior.
Basically we need to set the expectation that these are like TSRs and users should understand that ordering does matter.
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.
Added documentation to the hosting README with usage examples. Tweaked some of the XML comments for clarity. Allowed configuration callbacks to be registered inside themselves. Fixed some bugs. And, best of all, simplified things.